From ac000125214d73bbfe3306fc1ee5c714af39f7e4 Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Thu, 14 May 2026 13:01:26 -0700 Subject: [PATCH 1/7] SLING-13202 Remove dependency on commons-fileupload --- pom.xml | 6 - .../sling/engine/impl/SlingMainServlet.java | 20 +++ .../parameters/MultipartRequestParameter.java | 26 +-- .../engine/impl/parameters/ParameterMap.java | 10 +- .../impl/parameters/ParameterSupport.java | 143 ++++------------ .../parameters/RequestParameterConfig.java | 160 ++++++++++++++++++ .../RequestParameterSupportConfigurer.java | 153 ++--------------- .../impl/parameters/RequestPartsIterator.java | 99 +++++------ .../engine/impl/parameters/SlingPart.java | 98 ----------- .../engine/impl/SlingMainServletTest.java | 8 + 10 files changed, 303 insertions(+), 420 deletions(-) create mode 100644 src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java delete mode 100644 src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java diff --git a/pom.xml b/pom.xml index d23f99b9..d2686f7a 100644 --- a/pom.xml +++ b/pom.xml @@ -141,12 +141,6 @@ 2.1.2 provided - - commons-fileupload - commons-fileupload - 1.6.0 - provided - org.osgi org.osgi.framework diff --git a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java index 5df00c95..5f87935b 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java @@ -36,6 +36,7 @@ import org.apache.sling.engine.impl.helper.ClientAbortException; import org.apache.sling.engine.impl.helper.RequestListenerManager; import org.apache.sling.engine.impl.helper.SlingServletContext; +import org.apache.sling.engine.impl.parameters.RequestParameterConfig; import org.osgi.framework.BundleContext; import org.osgi.framework.Constants; import org.osgi.framework.ServiceRegistration; @@ -90,6 +91,10 @@ public class SlingMainServlet extends GenericServlet { @Reference private volatile SlingRequestProcessorImpl requestProcessorImpl; + /** extra config properties for multipart file upload support */ + @Reference + private volatile RequestParameterConfig reqParamConfig; + private volatile boolean allowTrace; private volatile ServiceRegistration servletRegistration; @@ -181,6 +186,21 @@ private Dictionary getServletContextRegistrationProps(final Stri if (servletName != null) { servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_NAME, servletName); } + + // configure support for multipart file uploads + servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_ENABLED, true); + // dig the multipart configuration out of the "Request Parameter Handling" configuration + final String multipartLocation = reqParamConfig.resolveLocation(); + if (multipartLocation != null) { + servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_LOCATION, multipartLocation); + } + final org.apache.sling.engine.impl.parameters.RequestParameterConfig.Config config = reqParamConfig.getConfig(); + servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_MAXFILESIZE, config.file_max()); + servletConfig.put( + HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_MAXREQUESTSIZE, config.request_max()); + servletConfig.put( + HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_FILESIZETHRESHOLD, config.file_threshold()); + servletConfig.put(Constants.SERVICE_DESCRIPTION, "Apache Sling Engine Main Servlet"); servletConfig.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation"); diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java b/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java index 46d448d7..42ee369f 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java @@ -20,9 +20,10 @@ import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import java.io.UnsupportedEncodingException; -import org.apache.commons.fileupload.disk.DiskFileItem; +import jakarta.servlet.http.Part; /** * The MultipartRequestParameter represents a request parameter @@ -34,14 +35,14 @@ */ public class MultipartRequestParameter extends AbstractRequestParameter { - private final DiskFileItem delegatee; + private final Part delegatee; private String encodedFileName; private String cachedValue; - public MultipartRequestParameter(DiskFileItem delegatee) { - super(delegatee.getFieldName(), null); + public MultipartRequestParameter(Part delegatee) { + super(delegatee.getName(), null); this.delegatee = delegatee; } @@ -49,7 +50,7 @@ void dispose() throws IOException { this.delegatee.delete(); } - DiskFileItem getFileItem() { + Part getPart() { return this.delegatee; } @@ -60,7 +61,11 @@ void setEncoding(String encoding) { } public byte[] get() { - return this.delegatee.get(); + try { + return getInputStream().readAllBytes(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } } public String getContentType() { @@ -72,8 +77,8 @@ public InputStream getInputStream() throws IOException { } public String getFileName() { - if (this.encodedFileName == null && this.delegatee.getName() != null) { - String tmpFileName = this.delegatee.getName(); + if (this.encodedFileName == null && this.delegatee.getSubmittedFileName() != null) { + String tmpFileName = this.delegatee.getSubmittedFileName(); if (this.getEncoding() != null) { try { byte[] rawName = tmpFileName.getBytes(Util.ENCODING_DIRECT); @@ -117,7 +122,7 @@ public String getString() { return this.cachedValue; } - return this.delegatee.getString(); + return new String(this.get()); } public String getString(String enc) throws UnsupportedEncodingException { @@ -125,7 +130,8 @@ public String getString(String enc) throws UnsupportedEncodingException { } public boolean isFormField() { - return this.delegatee.isFormField(); + // If getSubmittedFileName() returns null, it is likely a regular form field + return this.delegatee.getSubmittedFileName() == null; } public String toString() { diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java index 7f886c07..67dcffa8 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java @@ -125,8 +125,8 @@ public Map getStringParameterMap() { public Object getPart(final String name) { final RequestParameter p = this.getValue(name); - if (p instanceof MultipartRequestParameter) { - return new SlingPart((MultipartRequestParameter) p); + if (p instanceof MultipartRequestParameter mrp) { + return mrp.getPart(); } // no such part @@ -134,10 +134,10 @@ public Object getPart(final String name) { } public Collection getParts() { - final ArrayList parts = new ArrayList(this.size()); + final ArrayList parts = new ArrayList<>(this.size()); for (RequestParameter[] param : this.values()) { - if (param.length >= 1 && param[0] instanceof MultipartRequestParameter) { - parts.add(new SlingPart((MultipartRequestParameter) param[0])); + if (param.length >= 1 && param[0] instanceof MultipartRequestParameter mrp) { + parts.add(mrp.getPart()); } } return parts; diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java index 967b9ff5..fa4d344a 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java @@ -18,7 +18,6 @@ */ package org.apache.sling.engine.impl.parameters; -import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.UnsupportedEncodingException; @@ -29,14 +28,11 @@ import java.util.Locale; import java.util.Map; +import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletRequestWrapper; import jakarta.servlet.http.HttpServletResponse; -import org.apache.commons.fileupload.FileUploadException; -import org.apache.commons.fileupload.RequestContext; -import org.apache.commons.fileupload.disk.DiskFileItem; -import org.apache.commons.fileupload.disk.DiskFileItemFactory; -import org.apache.commons.fileupload.servlet.ServletFileUpload; +import jakarta.servlet.http.Part; import org.apache.sling.api.request.RequestParameter; import org.apache.sling.api.request.RequestParameterMap; import org.apache.sling.api.resource.ResourceResolver; @@ -69,6 +65,9 @@ public class ParameterSupport { /** Content type signaling parameters in request body */ private static final String WWW_FORM_URL_ENC = "application/x-www-form-urlencoded"; + /** Content type signaling parameters in multipart request body */ + private static final String MULTPART = "multipart/"; + /** name of the header used to identify an upload mode */ public static final String SLING_UPLOADMODE_HEADER = "Sling-uploadmode"; /** name of the parameter used to identify upload mode */ @@ -81,42 +80,12 @@ public class ParameterSupport { /** default log */ private final Logger log = LoggerFactory.getLogger(getClass()); - /** - * The maximum size allowed for multipart/form-data - * requests - * - *

The default is -1L, which means unlimited. - */ - private static long maxRequestSize = -1L; - - /** - * The directory location where files will be stored - */ - private static File location = null; - - /** - * The maximum size allowed for uploaded files. - * - *

The default is -1L, which means unlimited. - */ - private static long maxFileSize = -1L; - - /** - * The size threshold after which the file will be written to disk - */ - private static int fileSizeThreshold = 256000; - /** * Check for additional parameters from the container. * TODO - We can remove this once we move engine to Servlet 3.1 */ private static boolean checkForAdditionalParameters = false; - /** - * The maximum number of files allowed in a single request. - */ - private static long maxFileCount = 50; - private final HttpServletRequest servletRequest; private ParameterMap postParameterMap; @@ -157,19 +126,8 @@ public static HttpServletRequestWrapper getParameterSupportRequestWrapper(final return new ParameterSupportHttpServletRequestWrapper(request); } - static void configure( - final long maxRequestSize, - final String location, - final long maxFileSize, - final int fileSizeThreshold, - final boolean checkForAdditionalParameters, - final long maxFileCount) { - ParameterSupport.maxRequestSize = (maxRequestSize > 0) ? maxRequestSize : -1; - ParameterSupport.location = (location != null) ? new File(location) : null; - ParameterSupport.maxFileSize = (maxFileSize > 0) ? maxFileSize : -1; - ParameterSupport.fileSizeThreshold = (fileSizeThreshold > 0) ? fileSizeThreshold : 256000; + static void configure(final boolean checkForAdditionalParameters) { ParameterSupport.checkForAdditionalParameters = checkForAdditionalParameters; - ParameterSupport.maxFileCount = (maxFileCount > 0) ? maxFileCount : 50; } private ParameterSupport(HttpServletRequest servletRequest) { @@ -301,20 +259,17 @@ private ParameterMap getRequestParameterMapInternal() { } // Multipart POST - if (ServletFileUpload.isMultipartContent(this.getMultiPartContext())) { + if (isMultipartContent(this.getServletRequest())) { if (isStreamed(parameters, this.getServletRequest())) { // special case, the request is Multipart and streamed processing has been requested - try { - this.getServletRequest() - .setAttribute( - REQUEST_PARTS_ITERATOR_ATTRIBUTE, - new RequestPartsIterator(this.getMultiPartContext())); - this.log.debug( - "getRequestParameterMapInternal: Iterator available as request attribute named request-parts-iterator"); - } catch (final FileUploadException | IOException e) { - this.log.error( - "getRequestParameterMapInternal: Error parsing multipart streamed request", e); - } + this.getServletRequest() + .setAttribute( + REQUEST_PARTS_ITERATOR_ATTRIBUTE, + new RequestPartsIterator(this.getServletRequest())); + this.log.debug( + "getRequestParameterMapInternal: Iterator available as request attribute named {}", + REQUEST_PARTS_ITERATOR_ATTRIBUTE); + // The request data has been passed to the RequestPartsIterator, hence from a RequestParameter // pov its been used, and must not be used again. this.requestDataUsed = true; @@ -323,7 +278,12 @@ private ParameterMap getRequestParameterMapInternal() { addContainerParameters = false; useFallback = false; } else { - this.parseMultiPartPost(parameters); + try { + this.parseMultiPartPost(parameters); + } catch (final ServletException | IOException e) { + this.log.error( + "getRequestParameterMapInternal: Error parsing multipart streamed request", e); + } this.requestDataUsed = true; addContainerParameters = checkForAdditionalParameters; useFallback = false; @@ -390,55 +350,22 @@ private static final boolean isWWWFormEncodedContent(HttpServletRequest request) return false; } - private RequestContext getMultiPartContext() { - return new RequestContext() { - @Override - public String getCharacterEncoding() { - String enc = getServletRequest().getCharacterEncoding(); - return (enc != null) ? enc : Util.ENCODING_DIRECT; - } - - @Override - public int getContentLength() { - return getServletRequest().getContentLength(); - } - - @Override - public String getContentType() { - return getServletRequest().getContentType(); - } - - @Override - public InputStream getInputStream() throws IOException { - return getServletRequest().getInputStream(); - } - }; + /** + * Checks if the request is a multipart post + * + * @param request the request to check + * @return true if the request is multipart, false otherwise + */ + private static boolean isMultipartContent(HttpServletRequest request) { + final String contentType = request.getContentType(); + return contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith(MULTPART); } - private void parseMultiPartPost(ParameterMap parameters) { - // Create a new file upload handler - ServletFileUpload upload = new ServletFileUpload(); - upload.setSizeMax(ParameterSupport.maxRequestSize); - upload.setFileSizeMax(ParameterSupport.maxFileSize); - upload.setFileItemFactory( - new DiskFileItemFactory(ParameterSupport.fileSizeThreshold, ParameterSupport.location)); - upload.setFileCountMax(ParameterSupport.maxFileCount); - final RequestContext rc = this.getMultiPartContext(); - - // Parse the request - List /* FileItem */ items = null; - try { - items = upload.parseRequest(rc); - } catch (FileUploadException fue) { - this.log.error("parseMultiPartPost: Error parsing request", fue); - } - - if (items != null && items.size() > 0) { - for (Iterator ii = items.iterator(); ii.hasNext(); ) { - DiskFileItem fileItem = (DiskFileItem) ii.next(); - RequestParameter pp = new MultipartRequestParameter(fileItem); - parameters.addParameter(pp, false); - } + private void parseMultiPartPost(ParameterMap parameters) throws IOException, ServletException { + final Collection parts = this.getServletRequest().getParts(); + for (Part part : parts) { + RequestParameter pp = new MultipartRequestParameter(part); + parameters.addParameter(pp, false); } } } diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java new file mode 100644 index 00000000..6487077d --- /dev/null +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java @@ -0,0 +1,160 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.File; + +import org.apache.sling.engine.impl.SlingMainServlet; +import org.apache.sling.settings.SlingSettingsService; +import org.jetbrains.annotations.Nullable; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.osgi.service.metatype.annotations.AttributeDefinition; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Configuration component shared by {@link SlingMainServlet} and {@link RequestParameterSupportConfigurer} for + * backward compatibility of request parameter options + */ +@Component(name = RequestParameterConfig.PID, service = RequestParameterConfig.class) +@Designate(ocd = RequestParameterConfig.Config.class) +public class RequestParameterConfig { + static final String PID = "org.apache.sling.engine.parameters"; + + @ObjectClassDefinition( + name = "Apache Sling Request Parameter Handling", + description = "Configures Sling's request parameter handling.") + public @interface Config { + @AttributeDefinition( + name = "Default Parameter Encoding", + description = "The default request parameter encoding used to decode request " + + "parameters into strings. If this property is not set the default encoding " + + "is 'ISO-8859-1' as mandated by the Servlet API spec. This default encoding " + + "is used if the '_charset_' request parameter is not set to another " + + "(supported) character encoding. Applications being sure to always use the " + + "same encoding (e.g. UTF-8) can set this default here and may omit the " + + "'_charset_' request parameter") + String sling_default_parameter_encoding() default Util.ENCODING_DIRECT; + + @AttributeDefinition( + name = "Maximum POST Parameters", + description = "The maximum number of parameters supported. To prevent a DOS-style attack with an " + + "overrunning number of parameters the number of parameters supported can be limited. This " + + "includes all of the query string as well as application/x-www-form-urlencoded and " + + "multipart/form-data parameters. The default value is " + ParameterMap.DEFAULT_MAX_PARAMS + + ".") + int sling_default_max_parameters() default ParameterMap.DEFAULT_MAX_PARAMS; + + @AttributeDefinition( + name = "Temporary File Location", + description = "The temporary directory where uploaded files are written to disk. The default is " + + "null, which means the directory given by the 'java.io.tmpdir' system property.") + String file_location(); + + @AttributeDefinition( + name = "File Save Threshold", + description = "The size threshold after which the file will be written to disk. The default is 256KB.") + int file_threshold() default 256000; + + @AttributeDefinition( + name = "Maximum File Size", + description = "The maximum size allowed for uploaded files. The default is -1, which means unlimited.") + long file_max() default -1; + + @AttributeDefinition( + name = "Maximum Request Size", + description = + "The maximum size allowed for multipart/form-data requests. The default is -1, which means unlimited.") + long request_max() default -1; + + @AttributeDefinition( + name = "Check Additional Parameters", + description = + "Enable this if you want to include request parameters added through the container, e.g through a valve.") + boolean sling_default_parameter_checkForAdditionalContainerParameters() default false; + } + + /** default log */ + private final Logger log = LoggerFactory.getLogger(PID); + + @Reference + private SlingSettingsService settingsService; + + private Config config; + + @Activate + private void activate(Config config) { + this.config = config; + + if (log.isInfoEnabled()) { + final String fixEncoding = config.sling_default_parameter_encoding(); + final int maxParams = config.sling_default_max_parameters(); + final long maxRequestSize = config.request_max(); + final String fileLocation = resolveLocation(); + final long maxFileSize = config.file_max(); + final int fileSizeThreshold = config.file_threshold(); + final boolean checkAddParameters = config.sling_default_parameter_checkForAdditionalContainerParameters(); + + log.info("Default Character Encoding: {}", fixEncoding); + log.info("Parameter Number Limit: {}", (maxParams < 0) ? "unlimited" : maxParams); + log.info("Maximum Request Size: {}", (maxParams < 0) ? "unlimited" : maxRequestSize); + log.info("Temporary File Location: {}", fileLocation); + log.info("Maximum File Size: {}", maxFileSize); + log.info("Tempory File Creation Threshold: {}", fileSizeThreshold); + log.info("Check for additional container parameters: {}", checkAddParameters); + } + } + + public Config getConfig() { + return this.config; + } + + /** + * Resolve the file location for temp files + * @return the resolved file location + */ + public @Nullable String resolveLocation() { + String fileLocation = config.file_location(); + if (fileLocation != null) { + File file = new File(fileLocation); + if (!file.isAbsolute()) { + file = new File(this.settingsService.getSlingHomePath(), fileLocation); + fileLocation = file.getAbsolutePath(); + } + if (file.exists()) { + if (!file.isDirectory()) { + log.error( + "Configured temporary file location {} exists but is not a directory; using java.io.tmpdir instead", + fileLocation); + fileLocation = null; + } + } else { + if (!file.mkdirs()) { + log.error("Cannot create temporary file directory {}; using java.io.tmpdir instead", fileLocation); + fileLocation = null; + } + } + } + return fileLocation; + } +} diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java index 78586a36..2e6df036 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java @@ -18,179 +18,58 @@ */ package org.apache.sling.engine.impl.parameters; -import java.io.File; import java.io.IOException; import jakarta.servlet.Filter; import jakarta.servlet.FilterChain; -import jakarta.servlet.FilterConfig; import jakarta.servlet.ServletException; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; import jakarta.servlet.http.HttpServletRequest; import org.apache.sling.api.SlingJakartaHttpServletRequest; -import org.apache.sling.settings.SlingSettingsService; +import org.apache.sling.engine.impl.parameters.RequestParameterConfig.Config; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; import org.osgi.service.component.propertytypes.ServiceDescription; import org.osgi.service.component.propertytypes.ServiceRanking; import org.osgi.service.component.propertytypes.ServiceVendor; -import org.osgi.service.metatype.annotations.AttributeDefinition; -import org.osgi.service.metatype.annotations.Designate; -import org.osgi.service.metatype.annotations.ObjectClassDefinition; import org.osgi.service.servlet.whiteboard.propertytypes.HttpWhiteboardContextSelect; import org.osgi.service.servlet.whiteboard.propertytypes.HttpWhiteboardFilterPattern; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -@Component(name = RequestParameterSupportConfigurer.PID, service = Filter.class) +@Component(service = Filter.class) @HttpWhiteboardContextSelect("(osgi.http.whiteboard.context.name=org.apache.sling)") @HttpWhiteboardFilterPattern("/") @ServiceDescription("Filter for request parameter support") @ServiceVendor("The Apache Software Foundation") @ServiceRanking(Integer.MAX_VALUE) -@Designate(ocd = RequestParameterSupportConfigurer.Config.class) public class RequestParameterSupportConfigurer implements Filter { - @Override - public void init(FilterConfig filterConfig) throws ServletException {} - - @Override - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) - throws IOException, ServletException { - if (request instanceof HttpServletRequest - && !(request instanceof ParameterSupportHttpServletRequestWrapper) - && !(request instanceof SlingJakartaHttpServletRequest)) { - chain.doFilter(ParameterSupport.getParameterSupportRequestWrapper((HttpServletRequest) request), response); - } else { - chain.doFilter(request, response); - } - } - - @Override - public void destroy() {} - - @ObjectClassDefinition( - name = "Apache Sling Request Parameter Handling", - description = "Configures Sling's request parameter handling.") - public @interface Config { - @AttributeDefinition( - name = "Default Parameter Encoding", - description = "The default request parameter encoding used to decode request " - + "parameters into strings. If this property is not set the default encoding " - + "is 'ISO-8859-1' as mandated by the Servlet API spec. This default encoding " - + "is used if the '_charset_' request parameter is not set to another " - + "(supported) character encoding. Applications being sure to always use the " - + "same encoding (e.g. UTF-8) can set this default here and may omit the " - + "'_charset_' request parameter") - String sling_default_parameter_encoding() default Util.ENCODING_DIRECT; - - @AttributeDefinition( - name = "Maximum POST Parameters", - description = "The maximum number of parameters supported. To prevent a DOS-style attack with an " - + "overrunning number of parameters the number of parameters supported can be limited. This " - + "includes all of the query string as well as application/x-www-form-urlencoded and " - + "multipart/form-data parameters. The default value is " + ParameterMap.DEFAULT_MAX_PARAMS - + ".") - int sling_default_max_parameters() default ParameterMap.DEFAULT_MAX_PARAMS; - - @AttributeDefinition( - name = "Temporary File Location", - description = "The temporary directory where uploaded files are written to disk. The default is " - + "null, which means the directory given by the 'java.io.tmpdir' system property.") - String file_location(); - - @AttributeDefinition( - name = "File Save Threshold", - description = "The size threshold after which the file will be written to disk. The default is 256KB.") - int file_threshold() default 256000; - - @AttributeDefinition( - name = "Maximum File Size", - description = "The maximum size allowed for uploaded files. The default is -1, which means unlimited.") - long file_max() default -1; - - @AttributeDefinition( - name = "Maximum Request Size", - description = - "The maximum size allowed for multipart/form-data requests. The default is -1, which means unlimited.") - long request_max() default -1; - - @AttributeDefinition( - name = "Check Additional Parameters", - description = - "Enable this if you want to include request parameters added through the container, e.g through a valve.") - boolean sling_default_parameter_checkForAdditionalContainerParameters() default false; - - @AttributeDefinition( - name = "Maximum File Count", - description = - "The maximum number of files allowed for multipart/form-data requests in a single request. The default is 50.") - long request_max_file_count() default 50; - } - - static final String PID = "org.apache.sling.engine.parameters"; - - /** default log */ - private final Logger log = LoggerFactory.getLogger(PID); - + /** extra config properties for multipart file upload support */ @Reference - private SlingSettingsService settignsService; + private RequestParameterConfig reqParamConfig; @Activate - private void configure(final Config config) { + private void configure() { + final Config config = reqParamConfig.getConfig(); final String fixEncoding = config.sling_default_parameter_encoding(); final int maxParams = config.sling_default_max_parameters(); - final long maxRequestSize = config.request_max(); - final String fileLocation = getFileLocation(config.file_location()); - final long maxFileSize = config.file_max(); - final int fileSizeThreshold = config.file_threshold(); final boolean checkAddParameters = config.sling_default_parameter_checkForAdditionalContainerParameters(); - if (log.isInfoEnabled()) { - log.info("Default Character Encoding: {}", fixEncoding); - log.info("Parameter Number Limit: {}", (maxParams < 0) ? "unlimited" : maxParams); - log.info("Maximum Request Size: {}", (maxParams < 0) ? "unlimited" : maxRequestSize); - log.info("Temporary File Location: {}", fileLocation); - log.info("Maximum File Size: {}", maxFileSize); - log.info("Tempory File Creation Threshold: {}", fileSizeThreshold); - log.info("Check for additional container parameters: {}", checkAddParameters); - log.info("Maximum File Count: {}", config.request_max_file_count()); - } - Util.setDefaultFixEncoding(fixEncoding); ParameterMap.setMaxParameters(maxParams); - ParameterSupport.configure( - maxRequestSize, - fileLocation, - maxFileSize, - fileSizeThreshold, - checkAddParameters, - config.request_max_file_count()); + ParameterSupport.configure(checkAddParameters); } - private String getFileLocation(String fileLocation) { - if (fileLocation != null) { - File file = new File(fileLocation); - if (!file.isAbsolute()) { - file = new File(this.settignsService.getSlingHomePath(), fileLocation); - fileLocation = file.getAbsolutePath(); - } - if (file.exists()) { - if (!file.isDirectory()) { - log.error( - "Configured temporary file location {} exists but is not a directory; using java.io.tmpdir instead", - fileLocation); - fileLocation = null; - } - } else { - if (!file.mkdirs()) { - log.error("Cannot create temporary file directory {}; using java.io.tmpdir instead", fileLocation); - fileLocation = null; - } - } + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (request instanceof HttpServletRequest httpReq + && !(request instanceof ParameterSupportHttpServletRequestWrapper) + && !(request instanceof SlingJakartaHttpServletRequest)) { + chain.doFilter(ParameterSupport.getParameterSupportRequestWrapper(httpReq), response); + } else { + chain.doFilter(request, response); } - return fileLocation; } } diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java index f018b63f..7dd59354 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java @@ -20,61 +20,62 @@ import java.io.IOException; import java.io.InputStream; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Iterator; -import java.util.List; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.Part; -import org.apache.commons.fileupload.FileItemIterator; -import org.apache.commons.fileupload.FileItemStream; -import org.apache.commons.fileupload.FileUpload; -import org.apache.commons.fileupload.FileUploadException; -import org.apache.commons.fileupload.RequestContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Contains a Lazy iterator of Parts from the request stream loaded as the request is streamed using the Commons FileUpload API. + * Contains a Lazy iterator of Parts from the request stream loaded as the request is streamed */ public class RequestPartsIterator implements Iterator { - private static final Logger LOG = LoggerFactory.getLogger(RequestPartsIterator.class); + private final Logger log = LoggerFactory.getLogger(getClass()); - /** The CommonsFile Upload streaming API iterator */ - private final FileItemIterator itemIterator; + /** The Parts iterator */ + private Iterator itemIterator = null; + + /** supplier to retrieve the parts when needed */ + private final HttpServletRequest request; /** - * Create and initialse the iterator using the request. The request must be fresh. Headers can have been read but the stream - * must not have been parsed. - * @param servletRequest the request - * @throws IOException when there is a problem reading the request. - * @throws FileUploadException when there is a problem parsing the request. + * Create and initialize the iterator using the request. + * + * @param request the current request */ - public RequestPartsIterator(final RequestContext context) throws FileUploadException, IOException { - FileUpload upload = new FileUpload(); - upload.setFileCountMax(50); - itemIterator = upload.getItemIterator(context); + public RequestPartsIterator(final HttpServletRequest request) { + this.request = request; } @Override public boolean hasNext() { - try { - return itemIterator.hasNext(); - } catch (final FileUploadException | IOException e) { - LOG.error("hasNext Item failed cause:" + e.getMessage(), e); + // Use a lazy data iterator creation the first time it is needed to ensure + // the container doesn't parse the uploaded files until necessary + if (itemIterator == null) { + Collection parts; + try { + parts = request.getParts(); + } catch (ServletException | IOException e) { + log.error("Error parsing multipart streamed request", e); + parts = Collections.emptyList(); + } + + itemIterator = parts.iterator(); } - return false; + + return itemIterator.hasNext(); } @Override public Part next() { - try { - return new StreamedRequestPart(itemIterator.next()); - } catch (final FileUploadException | IOException e) { - LOG.error("next Item failed cause:" + e.getMessage(), e); + if (!hasNext()) { + throw new java.util.NoSuchElementException(); } - return null; + return new StreamedRequestPart(itemIterator.next()); } @Override @@ -83,35 +84,33 @@ public void remove() { } /** - * Internal implementation of the Part API from Servlet 3 wrapping the Commons File Upload FIleItemStream object. + * Internal implementation of the Part API from Servlet 3 wrapping the jakarta Part object. */ private static class StreamedRequestPart implements Part, javax.servlet.http.Part { - private final FileItemStream fileItem; - private final InputStream inputStream; + private final Part part; - public StreamedRequestPart(final FileItemStream fileItem) throws IOException { - this.fileItem = fileItem; - inputStream = fileItem.openStream(); + public StreamedRequestPart(final Part filePart) { + this.part = filePart; } @Override public InputStream getInputStream() throws IOException { - return inputStream; + return part.getInputStream(); } @Override public String getContentType() { - return fileItem.getContentType(); + return part.getContentType(); } @Override public String getName() { - return fileItem.getFieldName(); + return part.getName(); } @Override public long getSize() { - return 0; + return part.getSize(); } @Override @@ -127,34 +126,22 @@ public void delete() throws IOException { @Override public String getHeader(String headerName) { - return fileItem.getHeaders().getHeader(headerName); + return part.getHeader(headerName); } @Override public Collection getHeaders(String headerName) { - return toCollection(fileItem.getHeaders().getHeaders(headerName)); + return part.getHeaders(headerName); } @Override public Collection getHeaderNames() { - return toCollection(fileItem.getHeaders().getHeaderNames()); + return part.getHeaderNames(); } @Override public String getSubmittedFileName() { - return fileItem.getName(); - } - - private Collection toCollection(Iterator i) { - if (i == null) { - return Collections.emptyList(); - } else { - List c = new ArrayList(); - while (i.hasNext()) { - c.add(i.next()); - } - return c; - } + return part.getSubmittedFileName(); } } } diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java b/src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java deleted file mode 100644 index bf53bcd2..00000000 --- a/src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java +++ /dev/null @@ -1,98 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.sling.engine.impl.parameters; - -import java.io.IOException; -import java.io.InputStream; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Iterator; - -import jakarta.servlet.http.Part; - -public class SlingPart implements Part { - - private final MultipartRequestParameter param; - - public SlingPart(final MultipartRequestParameter param) { - this.param = param; - } - - @Override - public InputStream getInputStream() throws IOException { - return this.param.getInputStream(); - } - - @Override - public String getContentType() { - return this.param.getContentType(); - } - - @Override - public String getName() { - return this.param.getFileItem().getFieldName(); - } - - @Override - public long getSize() { - return this.param.getSize(); - } - - @Override - public void write(String fileName) throws IOException { - throw new IOException("Unsupported yet"); - } - - @Override - public void delete() { - this.param.getFileItem().delete(); - } - - @Override - public String getHeader(String name) { - return this.param.getFileItem().getHeaders().getHeader(name); - } - - @Override - public Collection getHeaders(String name) { - final ArrayList headers = new ArrayList(); - final Iterator itemHeaders = - this.param.getFileItem().getHeaders().getHeaders(name); - while (itemHeaders.hasNext()) { - headers.add(itemHeaders.next()); - } - return headers; - } - - @Override - public Collection getHeaderNames() { - final ArrayList headers = new ArrayList(); - final Iterator itemHeaders = - this.param.getFileItem().getHeaders().getHeaderNames(); - while (itemHeaders.hasNext()) { - headers.add(itemHeaders.next()); - } - return headers; - } - - @Override - public String getSubmittedFileName() { - return this.param.getFileName(); - } -} diff --git a/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java b/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java index 564fe561..76858c91 100644 --- a/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java +++ b/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java @@ -27,6 +27,8 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.apache.sling.engine.impl.parameters.RequestParameterConfig; +import org.apache.sling.settings.SlingSettingsService; import org.apache.sling.testing.mock.osgi.junit.OsgiContext; import org.junit.Before; import org.junit.Rule; @@ -66,6 +68,12 @@ public void setUp() { // Satisfy mandatory reference to SlingRequestProcessorImpl osgiContext.registerService(SlingRequestProcessorImpl.class, Mockito.mock(SlingRequestProcessorImpl.class)); + // Satisfy mandatory reference to RequestParameterConfig + osgiContext.registerService(SlingSettingsService.class, Mockito.mock(SlingSettingsService.class)); + + // Satisfy mandatory reference to RequestParameterConfig + osgiContext.registerInjectActivateService(RequestParameterConfig.class); + // Activate SlingMainServlet with OSGi config Map cfg = new HashMap<>(); cfg.put("sling_trace_allow", false); From 5aca587035c054a7d94997b48f2ffbba4ebd9c98 Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Thu, 14 May 2026 13:06:03 -0700 Subject: [PATCH 2/7] SLING-13202 cleanup comment --- .../java/org/apache/sling/engine/impl/SlingMainServletTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java b/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java index 76858c91..9a4f9fde 100644 --- a/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java +++ b/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java @@ -68,7 +68,7 @@ public void setUp() { // Satisfy mandatory reference to SlingRequestProcessorImpl osgiContext.registerService(SlingRequestProcessorImpl.class, Mockito.mock(SlingRequestProcessorImpl.class)); - // Satisfy mandatory reference to RequestParameterConfig + // Satisfy mandatory reference to SlingSettingsService osgiContext.registerService(SlingSettingsService.class, Mockito.mock(SlingSettingsService.class)); // Satisfy mandatory reference to RequestParameterConfig From bedd1f5d09d2a72302e3de4d01b434b3bd7661e6 Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Thu, 14 May 2026 15:41:14 -0700 Subject: [PATCH 3/7] SLING-13202 resolve sonar warnings and tests for code coverage --- .../sling/engine/impl/SlingMainServlet.java | 2 +- .../parameters/MultipartRequestParameter.java | 1 + .../parameters/RequestParameterConfig.java | 2 +- .../MultipartRequestParameterTest.java | 201 ++++++++++++++++++ .../RequestParameterConfigTest.java | 109 ++++++++++ .../parameters/RequestPartsIteratorTest.java | 134 ++++++++++++ 6 files changed, 447 insertions(+), 2 deletions(-) create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterConfigTest.java create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java diff --git a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java index 5f87935b..ca21f5f0 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java @@ -93,7 +93,7 @@ public class SlingMainServlet extends GenericServlet { /** extra config properties for multipart file upload support */ @Reference - private volatile RequestParameterConfig reqParamConfig; + private transient RequestParameterConfig reqParamConfig; private volatile boolean allowTrace; diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java b/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java index 42ee369f..fa48d13c 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java @@ -58,6 +58,7 @@ Part getPart() { void setEncoding(String encoding) { super.setEncoding(encoding); cachedValue = null; + encodedFileName = null; } public byte[] get() { diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java index 6487077d..fe753bb6 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java @@ -117,7 +117,7 @@ private void activate(Config config) { log.info("Default Character Encoding: {}", fixEncoding); log.info("Parameter Number Limit: {}", (maxParams < 0) ? "unlimited" : maxParams); - log.info("Maximum Request Size: {}", (maxParams < 0) ? "unlimited" : maxRequestSize); + log.info("Maximum Request Size: {}", (maxRequestSize < 0) ? "unlimited" : maxRequestSize); log.info("Temporary File Location: {}", fileLocation); log.info("Maximum File Size: {}", maxFileSize); log.info("Tempory File Creation Threshold: {}", fileSizeThreshold); diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java new file mode 100644 index 00000000..a8266675 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; + +import jakarta.servlet.http.Part; +import org.junit.Before; +import org.junit.Test; +import org.junit.Test.None; +import org.mockito.Mockito; + +import static org.junit.Assert.*; + +/** + * + */ +public class MultipartRequestParameterTest { + + private MultipartRequestParameter param; + private Part part1; + + @Before + public void before() { + part1 = Mockito.mock(Part.class); + param = new MultipartRequestParameter(part1); + } + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#setEncoding(java.lang.String)}. + */ + @Test + public void testSetEncoding() { + param.setEncoding("my/encoding1"); + assertEquals("my/encoding1", param.getEncoding()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#dispose()}. + */ + @Test(expected = None.class) + public void testDispose() throws IOException { + param.dispose(); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getPart()}. + */ + @Test + public void testGetPart() { + assertEquals(part1, param.getPart()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#get()}. + */ + @Test + public void testGet() throws IOException { + mockPartInputStream(); + + assertArrayEquals("hi".getBytes(), param.get()); + } + + @Test + public void testGetWithIOException() throws IOException { + InputStream inputStream = mockPartInputStream(); + Mockito.doThrow(IOException.class).when(inputStream).readAllBytes(); + + assertThrows(UncheckedIOException.class, param::get); + } + + private InputStream mockPartInputStream() throws IOException { + InputStream mockInputStream = Mockito.mock(InputStream.class); + Mockito.doReturn("hi".getBytes()).when(mockInputStream).readAllBytes(); + Mockito.doReturn(mockInputStream).when(part1).getInputStream(); + return mockInputStream; + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getContentType()}. + */ + @Test + public void testGetContentType() { + assertNull(param.getContentType()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getInputStream()}. + */ + @Test + public void testGetInputStream() throws IOException { + InputStream mockInputStream = mockPartInputStream(); + + assertEquals(mockInputStream, param.getInputStream()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getFileName()}. + */ + @Test + public void testGetFileName() { + assertNull(param.getFileName()); + + // simulate an uploaded file field + Mockito.doReturn("file1.txt").when(part1).getSubmittedFileName(); + + assertEquals("file1.txt", param.getFileName()); + // again for the cached value + assertEquals("file1.txt", param.getFileName()); + + // with some invalid encoding + param.setEncoding("invalid1"); + assertEquals("file1.txt", param.getFileName()); + + // with some valid encoding + param.setEncoding("UTF-8"); + assertEquals("file1.txt", param.getFileName()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getSize()}. + */ + @Test + public void testGetSize() { + assertEquals(0L, param.getSize()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getString()}. + */ + @Test + public void testGetString() throws IOException { + mockPartInputStream(); + assertNotNull(param.getString()); + // one more the for the cached value + assertNotNull(param.getString()); + + // with some invalid encoding + param.setEncoding("invalid1"); + assertNotNull(param.getString()); + + // with some valid encoding + param.setEncoding("UTF-8"); + assertNotNull(param.getString()); + + // simulate an uploaded file field + Mockito.doReturn("file1.txt").when(part1).getSubmittedFileName(); + assertNotNull(param.getString()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getString(java.lang.String)}. + */ + @Test + public void testGetStringString() throws IOException { + mockPartInputStream(); + assertNotNull(param.getString("UTF-8")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#isFormField()}. + */ + @Test + public void testIsFormField() { + assertTrue(param.isFormField()); + + // simulate an uploaded file field + Mockito.doReturn("file1.txt").when(part1).getSubmittedFileName(); + assertFalse(param.isFormField()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#toString()}. + */ + @Test + public void testToString() throws IOException { + mockPartInputStream(); + assertNotNull(param.toString()); + + // simulate an uploaded file field + Mockito.doReturn("file1.txt").when(part1).getSubmittedFileName(); + assertNotNull(param.toString()); + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterConfigTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterConfigTest.java new file mode 100644 index 00000000..c6b19edf --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterConfigTest.java @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; + +import org.apache.sling.settings.SlingSettingsService; +import org.apache.sling.testing.mock.osgi.junit.OsgiContext; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mockito; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +/** + * + */ +public class RequestParameterConfigTest { + @Rule + public final OsgiContext osgiContext = new OsgiContext(); + + private RequestParameterConfig config; + + private String tmpDir; + + @Before + public void before() { + // Satisfy mandatory reference to SlingSettingsService + SlingSettingsService mockSettingSvc = + osgiContext.registerService(SlingSettingsService.class, Mockito.mock(SlingSettingsService.class)); + tmpDir = System.getProperty("java.io.tmpdir"); + Mockito.doReturn(tmpDir).when(mockSettingSvc).getSlingHomePath(); + + config = osgiContext.registerInjectActivateService( + RequestParameterConfig.class, Map.of("sling.default.max.parameters", -1L, "request.max", 1000L)); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.RequestParameterConfig#getConfig()}. + */ + @Test + public void testGetConfig() { + assertNotNull(config.getConfig()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.RequestParameterConfig#resolveLocation()}. + */ + @Test + public void testResolveLocationWithNull() { + assertNull(config.resolveLocation()); + } + + @Test + public void testResolveLocationWithRelativePath() { + config = osgiContext.registerInjectActivateService( + RequestParameterConfig.class, Map.of("file.location", "temp")); + assertNotNull(config.resolveLocation()); + } + + @Test + public void testResolveLocationWithAbsoluteFolderPath() throws IOException { + final Path tempFile = Files.createTempDirectory("test"); + config = osgiContext.registerInjectActivateService( + RequestParameterConfig.class, + Map.of("file.location", tempFile.toFile().getAbsolutePath())); + assertNotNull(config.resolveLocation()); + } + + @Test + public void testResolveLocationWithAbsoluteFilePath() throws IOException { + final Path tempFile = Files.createTempFile("test", "test"); + config = osgiContext.registerInjectActivateService( + RequestParameterConfig.class, + Map.of("file.location", tempFile.toFile().getAbsolutePath())); + assertNull(config.resolveLocation()); + } + + @Test + public void testResolveLocationWithNotExistingPath() throws IOException { + Path tempFile = Files.createTempFile("test", "test"); + tempFile = tempFile.resolve("child1"); + config = osgiContext.registerInjectActivateService( + RequestParameterConfig.class, + Map.of("file.location", tempFile.toFile().getAbsolutePath())); + assertNull(config.resolveLocation()); + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java new file mode 100644 index 00000000..8da57a83 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; +import java.io.InputStream; +import java.util.List; +import java.util.NoSuchElementException; + +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.Part; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import static org.junit.Assert.*; + +/** + * + */ +public class RequestPartsIteratorTest { + + private RequestPartsIterator iterator; + private HttpServletRequest mockRequest; + + @Before + public void before() { + mockRequest = Mockito.mock(HttpServletRequest.class); + iterator = new RequestPartsIterator(mockRequest); + } + + private Part mockParts() throws IOException, ServletException { + Part part1 = Mockito.mock(Part.class); + Mockito.doReturn(List.of(part1)).when(mockRequest).getParts(); + return part1; + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.RequestPartsIterator#hasNext()}. + */ + @Test + public void testHasNextWithNoElements() { + assertFalse(iterator.hasNext()); + } + + @Test + public void testHasNextWithElements() throws IOException, ServletException { + mockParts(); + + assertTrue(iterator.hasNext()); + + iterator.next(); + assertFalse(iterator.hasNext()); + } + + @Test + public void testHasNextWithCaughtException() throws IOException, ServletException { + Mockito.doThrow(ServletException.class).when(mockRequest).getParts(); + assertFalse(iterator.hasNext()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.RequestPartsIterator#next()}. + */ + @Test + public void testNextWithNoElements() { + assertThrows(NoSuchElementException.class, iterator::next); + } + + @Test + public void testNextWithElements() throws IOException, ServletException { + mockParts(); + + final Part next1 = iterator.next(); + // the part implements both interfaces for backward compatibility + assertTrue(next1 instanceof javax.servlet.http.Part); + assertTrue(next1 instanceof jakarta.servlet.http.Part); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.RequestPartsIterator#remove()}. + */ + @Test + public void testRemove() { + assertThrows(UnsupportedOperationException.class, iterator::remove); + } + + @Test + public void testStreamedRequestPartWrapper() throws IOException, ServletException { + final Part part1 = mockParts(); + + InputStream mockInputStream1 = Mockito.mock(InputStream.class); + Mockito.doReturn(mockInputStream1).when(part1).getInputStream(); + Mockito.doReturn("text/test1").when(part1).getContentType(); + Mockito.doReturn("name1").when(part1).getName(); + Mockito.doReturn(11L).when(part1).getSize(); + Mockito.doReturn("value1").when(part1).getHeader("header1"); + Mockito.doReturn(List.of("value1")).when(part1).getHeaders("header1"); + Mockito.doReturn(List.of("header1")).when(part1).getHeaderNames(); + Mockito.doReturn("file1.txt").when(part1).getSubmittedFileName(); + + final Part next1 = iterator.next(); + + assertEquals(part1.getInputStream(), next1.getInputStream()); + assertEquals(part1.getContentType(), next1.getContentType()); + assertEquals(part1.getName(), next1.getName()); + assertEquals(part1.getSize(), next1.getSize()); + assertEquals(part1.getHeader("header"), next1.getHeader("header")); + assertEquals(part1.getHeaders("header"), next1.getHeaders("header")); + assertEquals(part1.getHeaderNames(), next1.getHeaderNames()); + assertEquals(part1.getSubmittedFileName(), next1.getSubmittedFileName()); + + assertThrows(UnsupportedOperationException.class, () -> next1.write("output1")); + // should do nothing + next1.delete(); + } +} From 39c4d729d9e5a95e7ed97cd526ca67a33feb172c Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Thu, 14 May 2026 16:56:51 -0700 Subject: [PATCH 4/7] SLING-13202 more tests for code coverage --- .../engine/impl/parameters/ParameterMap.java | 3 + .../impl/parameters/ParameterMapTest.java | 262 ++++++++++++++++++ ...RequestParameterSupportConfigurerTest.java | 98 +++++++ 3 files changed, 363 insertions(+) create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurerTest.java diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java index 67dcffa8..bfcae5f7 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java @@ -66,6 +66,7 @@ void renameParameter(String oldName, String newName) { } super.put(newName, params); + this.stringParameterMap = null; } void addParameter(RequestParameter parameter, boolean prependNew) { @@ -93,10 +94,12 @@ void addParameter(RequestParameter parameter, boolean prependNew) { // list of parameters this.requestParameters.add(parameter); + this.stringParameterMap = null; } void setParameters(String name, RequestParameter[] parameters) { super.put(name, parameters); + stringParameterMap = null; } // ---------- String parameter support diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java new file mode 100644 index 00000000..4e78c305 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java @@ -0,0 +1,262 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.util.List; +import java.util.Map; + +import jakarta.servlet.http.Part; +import org.apache.sling.api.request.RequestParameter; +import org.junit.Test; +import org.junit.Test.None; +import org.mockito.Mockito; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +/** + * + */ +public class ParameterMapTest { + + private ParameterMap map = new ParameterMap(); + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#clear()}. + */ + @Test + public void testClear() { + assertThrows(UnsupportedOperationException.class, map::clear); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#setMaxParameters(int)}. + */ + @Test(expected = None.class) + public void testSetMaxParameters() { + try { + ParameterMap.setMaxParameters(11); + ParameterMap.setMaxParameters(0); + } finally { + // restore the default + ParameterMap.setMaxParameters(ParameterMap.DEFAULT_MAX_PARAMS); + } + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getValue(java.lang.String)}. + */ + @Test + public void testGetValue() { + assertNull(map.getValue("key1")); + + map.setParameters("key1", new RequestParameter[0]); + assertNull(map.getValue("key1")); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + map.setParameters("key1", new RequestParameter[] {param1}); + assertEquals(param1, map.getValue("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getValues(java.lang.String)}. + */ + @Test + public void testGetValues() { + assertNull(map.getValues("key1")); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + final RequestParameter[] values1 = new RequestParameter[] {param1}; + map.setParameters("key1", values1); + assertArrayEquals(values1, map.getValues("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#renameParameter(java.lang.String, java.lang.String)}. + */ + @Test + public void testRenameParameter() { + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + map.setParameters("key1", new RequestParameter[] {param1}); + + map.renameParameter("key1", "key2"); + assertNull(map.getValue("key1")); + assertEquals(param1, map.getValue("key2")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#addParameter(org.apache.sling.api.request.RequestParameter, boolean)}. + */ + @Test + public void testAddParameter() { + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + map.addParameter(param1, true); + + assertEquals(param1, map.getValue("key1")); + + final ContainerRequestParameter param2 = new ContainerRequestParameter("key1", "value2", null); + map.addParameter(param2, false); + assertEquals(param1, map.getValue("key1")); + + final ContainerRequestParameter param3 = new ContainerRequestParameter("key1", "value3", null); + map.addParameter(param3, true); + assertEquals(param3, map.getValue("key1")); + + try { + ParameterMap.setMaxParameters(3); + final ContainerRequestParameter param4 = new ContainerRequestParameter("key1", "value4", null); + map.addParameter(param4, true); + assertEquals(param3, map.getValue("key1")); + } finally { + // restore the default + ParameterMap.setMaxParameters(ParameterMap.DEFAULT_MAX_PARAMS); + } + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#setParameters(java.lang.String, org.apache.sling.api.request.RequestParameter[])}. + */ + @Test + public void testSetParameters() { + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1a", "value1a", null); + final RequestParameter[] values1 = new RequestParameter[] {param1}; + map.setParameters("key1a", values1); + assertEquals("value1a", map.getStringValue("key1a")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getStringValue(java.lang.String)}. + */ + @Test + public void testGetStringValue() { + assertNull(map.getStringValue("key1")); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + final RequestParameter[] values1 = new RequestParameter[] {param1}; + map.setParameters("key1", values1); + assertEquals("value1", map.getStringValue("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getStringValues(java.lang.String)}. + */ + @Test + public void testGetStringValues() { + assertNull(map.getStringValues("key1")); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + final RequestParameter[] values1 = new RequestParameter[] {param1}; + map.setParameters("key1", values1); + assertArrayEquals(new String[] {"value1"}, map.getStringValues("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getStringParameterMap()}. + */ + @Test + public void testGetStringParameterMap() { + assertTrue(map.getStringParameterMap().isEmpty()); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + final RequestParameter[] values1 = new RequestParameter[] {param1}; + map.setParameters("key1", values1); + assertFalse(map.getStringParameterMap().isEmpty()); + // one more time for cached value coverage + assertFalse(map.getStringParameterMap().isEmpty()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getPart(java.lang.String)}. + */ + @Test + public void testGetPart() { + assertNull(map.getPart("key1")); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + map.addParameter(param1, true); + assertNull(map.getPart("key1")); + + Part part2 = Mockito.mock(Part.class); + Mockito.doReturn("key1").when(part2).getName(); + final MultipartRequestParameter param2 = new MultipartRequestParameter(part2); + map.addParameter(param2, true); + + assertEquals(part2, map.getPart("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getParts()}. + */ + @Test + public void testGetParts() { + assertTrue(map.getParts().isEmpty()); + + map.setParameters("key1", new RequestParameter[0]); + assertTrue(map.getParts().isEmpty()); + + final ContainerRequestParameter param1 = new ContainerRequestParameter("key1", "value1", null); + map.setParameters("key1", new RequestParameter[] {param1}); + assertTrue(map.getParts().isEmpty()); + + Part part2 = Mockito.mock(Part.class); + Mockito.doReturn("key1").when(part2).getName(); + final MultipartRequestParameter param2 = new MultipartRequestParameter(part2); + map.setParameters("key1", new RequestParameter[] {param2}); + + assertEquals(List.of(part2), map.getParts()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#getRequestParameterList()}. + */ + @Test + public void testGetRequestParameterList() { + assertTrue(map.getRequestParameterList().isEmpty()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#put(java.lang.String, org.apache.sling.api.request.RequestParameter[])}. + */ + @Test + public void testPutStringRequestParameterArray() { + final RequestParameter[] value = new RequestParameter[] {new ContainerRequestParameter("key1", "value1", null)}; + assertThrows(UnsupportedOperationException.class, () -> map.put("key1", value)); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#putAll(java.util.Map)}. + */ + @Test + public void testPutAllMapOfQextendsStringQextendsRequestParameter() { + Map map1 = Map.of(); + assertThrows(UnsupportedOperationException.class, () -> map.putAll(map1)); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterMap#remove(java.lang.Object)}. + */ + @Test + public void testRemoveObject() { + assertThrows(UnsupportedOperationException.class, () -> map.remove("key1")); + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurerTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurerTest.java new file mode 100644 index 00000000..abe32aec --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurerTest.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; +import org.apache.sling.api.SlingJakartaHttpServletRequest; +import org.apache.sling.settings.SlingSettingsService; +import org.apache.sling.testing.mock.osgi.junit.OsgiContext; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mockito; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.times; + +/** + * + */ +public class RequestParameterSupportConfigurerTest { + + @Rule + public final OsgiContext osgiContext = new OsgiContext(); + + private RequestParameterSupportConfigurer filter; + + @Before + public void before() { + // Satisfy mandatory reference to SlingSettingsService + osgiContext.registerService(SlingSettingsService.class, Mockito.mock(SlingSettingsService.class)); + + // Satisfy mandatory reference to RequestParameterConfig + osgiContext.registerInjectActivateService(RequestParameterConfig.class); + + filter = osgiContext.registerInjectActivateService(RequestParameterSupportConfigurer.class); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.RequestParameterSupportConfigurer#doFilter(jakarta.servlet.ServletRequest, jakarta.servlet.ServletResponse, jakarta.servlet.FilterChain)}. + */ + @Test + public void testDoFilterForServletRequest() throws IOException, ServletException { + ServletRequest mockRequest1 = Mockito.mock(ServletRequest.class); + doFilterForServletRequest(mockRequest1); + } + + @Test + public void testDoFilterForParameterSupportHttpServletRequestWrapper() throws IOException, ServletException { + ParameterSupportHttpServletRequestWrapper mockRequest1 = + Mockito.mock(ParameterSupportHttpServletRequestWrapper.class); + doFilterForServletRequest(mockRequest1); + } + + @Test + public void testDoFilterForSlingJakartaHttpServletRequest() throws IOException, ServletException { + SlingJakartaHttpServletRequest mockRequest1 = Mockito.mock(SlingJakartaHttpServletRequest.class); + doFilterForServletRequest(mockRequest1); + } + + private void doFilterForServletRequest(ServletRequest mockRequest1) throws IOException, ServletException { + ServletResponse mockResponse1 = Mockito.mock(ServletResponse.class); + FilterChain chain1 = Mockito.mock(FilterChain.class); + filter.doFilter(mockRequest1, mockResponse1, chain1); + Mockito.verify(chain1, times(1)).doFilter(any(ServletRequest.class), any(ServletResponse.class)); + } + + @Test + public void testDoFilterForHttpServletRequest() throws IOException, ServletException { + HttpServletRequest mockRequest1 = Mockito.mock(HttpServletRequest.class); + ServletResponse mockResponse1 = Mockito.mock(ServletResponse.class); + FilterChain chain1 = Mockito.mock(FilterChain.class); + filter.doFilter(mockRequest1, mockResponse1, chain1); + Mockito.verify(chain1, times(1)).doFilter(any(HttpServletRequestWrapper.class), any(ServletResponse.class)); + } +} From 4f84a1806860fed72df888857bab45e9b386b34e Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Fri, 15 May 2026 09:35:22 -0700 Subject: [PATCH 5/7] SLING-13202 even more tests for code coverage --- .../impl/parameters/ParameterSupport.java | 8 +- .../impl/parameters/ParameterSupportTest.java | 364 ++++++++++++++++++ 2 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java index fa4d344a..a9890e93 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java @@ -225,11 +225,9 @@ private ParameterMap getRequestParameterMapInternal() { InputStream input = Util.toInputStream(query); Util.parseQueryString(input, encoding, parameters, false); addContainerParameters = checkForAdditionalParameters; - } catch (IllegalArgumentException e) { - this.log.error("getRequestParameterMapInternal: Error parsing request", e); } catch (UnsupportedEncodingException e) { throw new SlingUnsupportedEncodingException(e); - } catch (IOException e) { + } catch (IllegalArgumentException | IOException e) { this.log.error("getRequestParameterMapInternal: Error parsing request", e); } useFallback = false; @@ -247,11 +245,9 @@ private ParameterMap getRequestParameterMapInternal() { InputStream input = this.getServletRequest().getInputStream(); Util.parseQueryString(input, encoding, parameters, false); addContainerParameters = checkForAdditionalParameters; - } catch (IllegalArgumentException e) { - this.log.error("getRequestParameterMapInternal: Error parsing request", e); } catch (UnsupportedEncodingException e) { throw new SlingUnsupportedEncodingException(e); - } catch (IOException e) { + } catch (IllegalArgumentException | IOException e) { this.log.error("getRequestParameterMapInternal: Error parsing request", e); } this.requestDataUsed = true; diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java new file mode 100644 index 00000000..c3a6ff41 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java @@ -0,0 +1,364 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; + +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.Part; +import org.apache.sling.api.request.RequestParameter; +import org.apache.sling.api.request.RequestParameterMap; +import org.junit.Before; +import org.junit.Test; +import org.junit.Test.None; +import org.mockito.Mockito; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.times; + +/** + * + */ +public class ParameterSupportTest { + + private ParameterSupport support; + private HttpServletRequest mockRequest; + + @Before + public void before() { + mockRequest = Mockito.mock(HttpServletRequest.class); + support = ParameterSupport.getInstance(mockRequest); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getInstance(jakarta.servlet.http.HttpServletRequest)}. + */ + @Test + public void testGetInstance() { + String attrName = ParameterSupport.class.getName(); + final ParameterSupport support1 = ParameterSupport.getInstance(mockRequest); + assertNotNull(support1); + Mockito.verify(mockRequest, times(1)).setAttribute(attrName, support1); + + // cover cached value in request attr + Mockito.doReturn(support1).when(mockRequest).getAttribute(attrName); + assertEquals(support1, ParameterSupport.getInstance(mockRequest)); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getParameterSupportRequestWrapper(jakarta.servlet.http.HttpServletRequest)}. + */ + @Test + public void testGetParameterSupportRequestWrapper() { + assertNotNull(ParameterSupport.getParameterSupportRequestWrapper(mockRequest)); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#configure(boolean)}. + */ + @Test(expected = None.class) + public void testConfigure() { + ParameterSupport.configure(true); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#requestDataUsed()}. + */ + @Test + public void testRequestDataUsed() { + assertFalse(support.requestDataUsed()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getParameter(java.lang.String)}. + */ + @Test + public void testGetParameter() throws IOException { + mockFormEncodedPost(); + assertEquals("value1", support.getParameter("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getParameterValues(java.lang.String)}. + */ + @Test + public void testGetParameterValues() throws IOException, ServletException { + mockMultipartPost(); + assertArrayEquals(new String[] {"value1"}, support.getParameterValues("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getParameterMap()}. + */ + @Test + public void testGetParameterMap() throws IOException { + mockFormEncodedPost(); + final Map parameterMap = support.getParameterMap(); + assertEquals(4, parameterMap.size()); + assertArrayEquals(new String[] {"value1"}, parameterMap.get("key1")); + assertArrayEquals(new String[] {"qvalue2"}, parameterMap.get("qkey1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getParameterNames()}. + */ + @Test + public void testGetParameterNames() throws IOException { + mockFormEncodedPost(); + final Enumeration parameterNames = support.getParameterNames(); + assertNotNull(parameterNames); + assertTrue(parameterNames.hasMoreElements()); + assertNotNull(parameterNames.nextElement()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getRequestParameter(java.lang.String)}. + */ + @Test + public void testGetRequestParameter() throws IOException { + mockFormEncodedPost(); + assertNotNull(support.getRequestParameter("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getRequestParameters(java.lang.String)}. + */ + @Test + public void testGetRequestParameters() throws IOException { + mockFormEncodedPost(); + assertNotNull(support.getRequestParameters("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getPart(java.lang.String)}. + */ + @Test + public void testGetPart() throws IOException, ServletException { + mockMultipartPost(); + assertNotNull(support.getPart("key1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getParts()}. + */ + @Test + public void testGetParts() throws IOException, ServletException { + mockMultipartPost(); + assertEquals(2, support.getParts().size()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getRequestParameterMap()}. + */ + @Test + public void testGetRequestParameterMap() throws IOException { + mockFormEncodedPost(); + final RequestParameterMap map1 = support.getRequestParameterMap(); + assertTrue(map1.containsKey("key1")); + assertEquals("value1", map1.getValue("key1").getString()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.ParameterSupport#getRequestParameterList()}. + */ + @Test + public void testGetRequestParameterList() throws IOException { + mockFormEncodedPost(); + final List list1 = support.getRequestParameterList(); + assertEquals(4, list1.size()); + } + + @Test + public void testMultipartPost() throws IOException, ServletException { + mockMultipartPost(); + final List list1 = support.getRequestParameterList(); + assertEquals(4, list1.size()); + } + + @Test + public void testStreamedMultipartPost1() throws IOException, ServletException { + mockMultipartPost(); + Mockito.doReturn(ParameterSupport.STREAM_UPLOAD) + .when(mockRequest) + .getHeader(ParameterSupport.SLING_UPLOADMODE_HEADER); + assertStreamedMultipartPost(); + } + + @Test + public void testStreamedMultipartPost2() throws IOException, ServletException { + mockMultipartPost(); + Mockito.doReturn("uploadmode=stream&qkey2=qvalue2").when(mockRequest).getQueryString(); + assertStreamedMultipartPost(); + } + + @SuppressWarnings("unchecked") + protected void assertStreamedMultipartPost() { + AtomicReference> partsIt = new AtomicReference<>(); + Mockito.doAnswer(invocation -> { + partsIt.set(invocation.getArgument(1, Iterator.class)); + return null; + }) + .when(mockRequest) + .setAttribute(eq(ParameterSupport.REQUEST_PARTS_ITERATOR_ATTRIBUTE), any(Iterator.class)); + final List list1 = support.getRequestParameterList(); + assertEquals(2, list1.size()); + + final Iterator iterator = partsIt.get(); + assertNotNull(iterator); + assertTrue(iterator.hasNext()); + assertNotNull(iterator.next()); + } + + @Test + public void testPostWithFallbackToContainerParams() { + mockFallbackPost(); + Mockito.doReturn(null).when(mockRequest).getContentType(); + final List list1 = support.getRequestParameterList(); + assertEquals(2, list1.size()); + } + + @Test + public void testUnsupportedCharacterEncoding1() throws IOException { + mockFormEncodedPost(); + Mockito.doReturn(null).when(mockRequest).getCharacterEncoding(); + Mockito.doThrow(UnsupportedEncodingException.class) + .when(mockRequest) + .setCharacterEncoding(Util.ENCODING_DIRECT); + + assertThrows(SlingUnsupportedEncodingException.class, support::getParameterMap); + } + + @Test + public void testUnsupportedCharacterEncoding2() throws IOException { + mockFormEncodedPost(); + Mockito.doReturn("invalid").when(mockRequest).getCharacterEncoding(); + + assertThrows(SlingUnsupportedEncodingException.class, support::getParameterMap); + } + + @Test + public void testUnsupportedCharacterEncoding3() throws IOException { + mockFormEncodedPost(); + Mockito.doReturn("invalid").when(mockRequest).getCharacterEncoding(); + Mockito.doReturn(null).when(mockRequest).getQueryString(); + + assertThrows(SlingUnsupportedEncodingException.class, support::getParameterMap); + } + + private void mockFormEncodedPost() throws IOException { + Mockito.doReturn("POST").when(mockRequest).getMethod(); + Mockito.doReturn("application/x-www-form-urlencoded").when(mockRequest).getContentType(); + Mockito.doReturn("qkey1=qvalue2&qkey2=qvalue2").when(mockRequest).getQueryString(); + + Map containerParams = new HashMap<>(); + containerParams.put("key1", new String[] {"value1"}); + containerParams.put("key2", new String[] {"value2"}); + Mockito.doReturn(containerParams).when(mockRequest).getParameterMap(); + + Map params = new HashMap<>(); + params.put("key1", "value1"); + params.put("key2", "value2"); + + String formUrlEncoded = params.entrySet().stream() + .map(e -> URLEncoder.encode(e.getKey(), StandardCharsets.UTF_8) + "=" + + URLEncoder.encode(e.getValue(), StandardCharsets.UTF_8)) + .collect(Collectors.joining("&")); + + ServletInputStream inStream = new ServletInputStream() { + private final InputStream is = new ByteArrayInputStream(formUrlEncoded.getBytes(StandardCharsets.UTF_8)); + + @Override + public int read() throws IOException { + return is.read(); + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public boolean isFinished() { + throw new UnsupportedOperationException(); + } + + @Override + public void setReadListener(ReadListener readListener) { + throw new UnsupportedOperationException(); + } + }; + Mockito.doReturn(inStream).when(mockRequest).getInputStream(); + } + + private void mockMultipartPost() throws IOException, ServletException { + Mockito.doReturn("POST").when(mockRequest).getMethod(); + Mockito.doReturn("multipart/form-data").when(mockRequest).getContentType(); + Mockito.doReturn("qkey1=qvalue2&qkey2=qvalue2").when(mockRequest).getQueryString(); + + Map containerParams = new HashMap<>(); + containerParams.put("key1", new String[] {"value1"}); + containerParams.put("key2", new String[] {"value2"}); + Mockito.doReturn(containerParams).when(mockRequest).getParameterMap(); + + Part part1 = Mockito.mock(Part.class); + Mockito.doReturn("key1").when(part1).getName(); + Mockito.doReturn(new ByteArrayInputStream("value1".getBytes())) + .when(part1) + .getInputStream(); + Part part2 = Mockito.mock(Part.class); + Mockito.doReturn("key2").when(part2).getName(); + Mockito.doReturn(new ByteArrayInputStream("value2".getBytes())) + .when(part2) + .getInputStream(); + Mockito.doReturn(List.of(part1, part2)).when(mockRequest).getParts(); + } + + private void mockFallbackPost() { + Mockito.doReturn("POST").when(mockRequest).getMethod(); + Mockito.doReturn(null).when(mockRequest).getContentType(); + Mockito.doReturn(null).when(mockRequest).getQueryString(); + + Map containerParams = new HashMap<>(); + containerParams.put("key1", new String[] {"value1"}); + containerParams.put("key2", new String[] {"value2"}); + Mockito.doReturn(containerParams).when(mockRequest).getParameterMap(); + } +} From 6efb03f8fc362719fcc27e32fa79d424ae4b084d Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Sat, 16 May 2026 18:35:36 -0700 Subject: [PATCH 6/7] SLING-13202 incorporate changes requested by code review --- .../FileCountLimitExceededException.java | 55 +++++++ .../parameters/MultipartRequestParameter.java | 23 ++- .../engine/impl/parameters/ParameterMap.java | 4 +- .../impl/parameters/ParameterSupport.java | 29 ++-- .../parameters/RequestParameterConfig.java | 8 + .../RequestParameterSupportConfigurer.java | 3 +- .../impl/parameters/RequestPartsIterator.java | 24 ++- .../engine/impl/parameters/SlingPart.java | 84 ++++++++++ .../FileCountLimitExceededExceptionTest.java | 42 +++++ .../MultipartRequestParameterTest.java | 27 +++- .../impl/parameters/ParameterMapTest.java | 12 +- .../impl/parameters/ParameterSupportTest.java | 29 +++- .../parameters/RequestPartsIteratorTest.java | 16 +- .../engine/impl/parameters/SlingPartTest.java | 145 ++++++++++++++++++ 14 files changed, 466 insertions(+), 35 deletions(-) create mode 100644 src/main/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededException.java create mode 100644 src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededExceptionTest.java create mode 100644 src/test/java/org/apache/sling/engine/impl/parameters/SlingPartTest.java diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededException.java b/src/main/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededException.java new file mode 100644 index 00000000..d3ba3f26 --- /dev/null +++ b/src/main/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededException.java @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; + +/** + * This exception is thrown if a request contains more files than the specified + * limit. + */ +public class FileCountLimitExceededException extends IOException { + + private static final long serialVersionUID = 6904179610227521789L; + + /** + * The limit that was exceeded. + */ + private final long limit; + + /** + * Creates a new instance. + * + * @param message The detail message + * @param limit The limit that was exceeded + */ + public FileCountLimitExceededException(final String message, final long limit) { + super(message); + this.limit = limit; + } + + /** + * Gets the limit that was exceeded. + * + * @return The limit that was exceeded by the request + */ + public long getLimit() { + return limit; + } +} diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java b/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java index fa48d13c..e71fa7db 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java @@ -22,6 +22,7 @@ import java.io.InputStream; import java.io.UncheckedIOException; import java.io.UnsupportedEncodingException; +import java.nio.charset.StandardCharsets; import jakarta.servlet.http.Part; @@ -38,11 +39,10 @@ public class MultipartRequestParameter extends AbstractRequestParameter { private final Part delegatee; private String encodedFileName; - private String cachedValue; - public MultipartRequestParameter(Part delegatee) { - super(delegatee.getName(), null); + public MultipartRequestParameter(Part delegatee, String encoding) { + super(delegatee.getName(), encoding); this.delegatee = delegatee; } @@ -62,8 +62,8 @@ void setEncoding(String encoding) { } public byte[] get() { - try { - return getInputStream().readAllBytes(); + try (InputStream inStream = getInputStream()) { + return inStream.readAllBytes(); } catch (IOException e) { throw new UncheckedIOException(e); } @@ -123,7 +123,18 @@ public String getString() { return this.cachedValue; } - return new String(this.get()); + // try explicit encoding if available + String encoding = getEncoding(); + if (encoding == null) { + // fallback to be used when no explicit value is provided + encoding = StandardCharsets.ISO_8859_1.name(); + } + try { + return new String(this.get(), encoding); + } catch (final UnsupportedEncodingException e) { + // don't care, fall back to empty for backward compatibility + return ""; + } } public String getString(String enc) throws UnsupportedEncodingException { diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java index bfcae5f7..4471946e 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java @@ -129,7 +129,7 @@ public Map getStringParameterMap() { public Object getPart(final String name) { final RequestParameter p = this.getValue(name); if (p instanceof MultipartRequestParameter mrp) { - return mrp.getPart(); + return new SlingPart(mrp); } // no such part @@ -140,7 +140,7 @@ public Collection getParts() { final ArrayList parts = new ArrayList<>(this.size()); for (RequestParameter[] param : this.values()) { if (param.length >= 1 && param[0] instanceof MultipartRequestParameter mrp) { - parts.add(mrp.getPart()); + parts.add(new SlingPart(mrp)); } } return parts; diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java index a9890e93..b3349308 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java @@ -66,7 +66,7 @@ public class ParameterSupport { private static final String WWW_FORM_URL_ENC = "application/x-www-form-urlencoded"; /** Content type signaling parameters in multipart request body */ - private static final String MULTPART = "multipart/"; + private static final String MULTIPART = "multipart/"; /** name of the header used to identify an upload mode */ public static final String SLING_UPLOADMODE_HEADER = "Sling-uploadmode"; @@ -86,6 +86,11 @@ public class ParameterSupport { */ private static boolean checkForAdditionalParameters = false; + /** + * The maximum number of files allowed in a single request. + */ + private static long maxFileCount = 50; + private final HttpServletRequest servletRequest; private ParameterMap postParameterMap; @@ -126,8 +131,9 @@ public static HttpServletRequestWrapper getParameterSupportRequestWrapper(final return new ParameterSupportHttpServletRequestWrapper(request); } - static void configure(final boolean checkForAdditionalParameters) { + static void configure(final boolean checkForAdditionalParameters, final long maxFileCount) { ParameterSupport.checkForAdditionalParameters = checkForAdditionalParameters; + ParameterSupport.maxFileCount = (maxFileCount > 0) ? maxFileCount : 50; } private ParameterSupport(HttpServletRequest servletRequest) { @@ -261,7 +267,8 @@ private ParameterMap getRequestParameterMapInternal() { this.getServletRequest() .setAttribute( REQUEST_PARTS_ITERATOR_ATTRIBUTE, - new RequestPartsIterator(this.getServletRequest())); + new RequestPartsIterator( + this.getServletRequest(), ParameterSupport.maxFileCount)); this.log.debug( "getRequestParameterMapInternal: Iterator available as request attribute named {}", REQUEST_PARTS_ITERATOR_ATTRIBUTE); @@ -275,10 +282,9 @@ private ParameterMap getRequestParameterMapInternal() { useFallback = false; } else { try { - this.parseMultiPartPost(parameters); + this.parseMultiPartPost(parameters, encoding); } catch (final ServletException | IOException e) { - this.log.error( - "getRequestParameterMapInternal: Error parsing multipart streamed request", e); + this.log.error("getRequestParameterMapInternal: Error parsing multipart request", e); } this.requestDataUsed = true; addContainerParameters = checkForAdditionalParameters; @@ -354,13 +360,18 @@ private static final boolean isWWWFormEncodedContent(HttpServletRequest request) */ private static boolean isMultipartContent(HttpServletRequest request) { final String contentType = request.getContentType(); - return contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith(MULTPART); + return contentType != null && contentType.toLowerCase(Locale.ENGLISH).startsWith(MULTIPART); } - private void parseMultiPartPost(ParameterMap parameters) throws IOException, ServletException { + private void parseMultiPartPost(ParameterMap parameters, String encoding) throws IOException, ServletException { final Collection parts = this.getServletRequest().getParts(); + long filePartCount = + parts.stream().filter(p -> p.getSubmittedFileName() != null).count(); + if (filePartCount > maxFileCount) { + throw new FileCountLimitExceededException("Request exceeds maximum file count", maxFileCount); + } for (Part part : parts) { - RequestParameter pp = new MultipartRequestParameter(part); + RequestParameter pp = new MultipartRequestParameter(part, encoding); parameters.addParameter(pp, false); } } diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java index fe753bb6..862b1fad 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterConfig.java @@ -92,6 +92,12 @@ public class RequestParameterConfig { description = "Enable this if you want to include request parameters added through the container, e.g through a valve.") boolean sling_default_parameter_checkForAdditionalContainerParameters() default false; + + @AttributeDefinition( + name = "Maximum File Count", + description = + "The maximum number of files allowed for multipart/form-data requests in a single request. The default is 50.") + long request_max_file_count() default 50; } /** default log */ @@ -114,6 +120,7 @@ private void activate(Config config) { final long maxFileSize = config.file_max(); final int fileSizeThreshold = config.file_threshold(); final boolean checkAddParameters = config.sling_default_parameter_checkForAdditionalContainerParameters(); + final long maxFileCount = config.request_max_file_count(); log.info("Default Character Encoding: {}", fixEncoding); log.info("Parameter Number Limit: {}", (maxParams < 0) ? "unlimited" : maxParams); @@ -122,6 +129,7 @@ private void activate(Config config) { log.info("Maximum File Size: {}", maxFileSize); log.info("Tempory File Creation Threshold: {}", fileSizeThreshold); log.info("Check for additional container parameters: {}", checkAddParameters); + log.info("Maximum File Count: {}", maxFileCount); } } diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java index 2e6df036..be8569b7 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestParameterSupportConfigurer.java @@ -55,10 +55,11 @@ private void configure() { final String fixEncoding = config.sling_default_parameter_encoding(); final int maxParams = config.sling_default_max_parameters(); final boolean checkAddParameters = config.sling_default_parameter_checkForAdditionalContainerParameters(); + final long maxFileCount = config.request_max_file_count(); Util.setDefaultFixEncoding(fixEncoding); ParameterMap.setMaxParameters(maxParams); - ParameterSupport.configure(checkAddParameters); + ParameterSupport.configure(checkAddParameters, maxFileCount); } @Override diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java index 7dd59354..6bd70d2e 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java @@ -42,13 +42,16 @@ public class RequestPartsIterator implements Iterator { /** supplier to retrieve the parts when needed */ private final HttpServletRequest request; + private final long maxFileCount; + /** * Create and initialize the iterator using the request. * * @param request the current request */ - public RequestPartsIterator(final HttpServletRequest request) { + public RequestPartsIterator(final HttpServletRequest request, long maxFileCount) { this.request = request; + this.maxFileCount = maxFileCount; } @Override @@ -58,7 +61,7 @@ public boolean hasNext() { if (itemIterator == null) { Collection parts; try { - parts = request.getParts(); + parts = getPartsAndCheckFileCount(); } catch (ServletException | IOException e) { log.error("Error parsing multipart streamed request", e); parts = Collections.emptyList(); @@ -70,6 +73,21 @@ public boolean hasNext() { return itemIterator.hasNext(); } + /** + * Get the parts from the request and check if the number of files exceeds the allowed number + * @return the collected parts + */ + private Collection getPartsAndCheckFileCount() throws IOException, ServletException { + Collection parts = request.getParts(); + + long filePartCount = + parts.stream().filter(p -> p.getSubmittedFileName() != null).count(); + if (filePartCount > maxFileCount) { + throw new FileCountLimitExceededException("Request exceeds maximum file count", maxFileCount); + } + return parts; + } + @Override public Part next() { if (!hasNext()) { @@ -121,7 +139,7 @@ public void write(String s) throws IOException { @Override public void delete() throws IOException { - // no underlying storage is used, so nothing to delete. + part.delete(); } @Override diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java b/src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java new file mode 100644 index 00000000..3f9aa5f8 --- /dev/null +++ b/src/main/java/org/apache/sling/engine/impl/parameters/SlingPart.java @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Collection; + +import jakarta.servlet.http.Part; + +public class SlingPart implements Part { + + private final MultipartRequestParameter param; + + public SlingPart(final MultipartRequestParameter param) { + this.param = param; + } + + @Override + public InputStream getInputStream() throws IOException { + return this.param.getInputStream(); + } + + @Override + public String getContentType() { + return this.param.getContentType(); + } + + @Override + public String getName() { + return this.param.getPart().getName(); + } + + @Override + public long getSize() { + return this.param.getSize(); + } + + @Override + public void write(String fileName) throws IOException { + throw new IOException("Unsupported yet"); + } + + @Override + public void delete() throws IOException { + this.param.getPart().delete(); + } + + @Override + public String getHeader(String name) { + return this.param.getPart().getHeader(name); + } + + @Override + public Collection getHeaders(String name) { + return this.param.getPart().getHeaders(name); + } + + @Override + public Collection getHeaderNames() { + return this.param.getPart().getHeaderNames(); + } + + @Override + public String getSubmittedFileName() { + return this.param.getFileName(); + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededExceptionTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededExceptionTest.java new file mode 100644 index 00000000..dc18ffb9 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/FileCountLimitExceededExceptionTest.java @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import org.junit.Test; + +import static org.junit.Assert.*; + +/** + * + */ +public class FileCountLimitExceededExceptionTest { + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.FileCountLimitExceededExceptionTest#getLimit()}. + */ + @Test + public void testGetLimit() { + try { + throw new FileCountLimitExceededException("failure1", 3L); + } catch (FileCountLimitExceededException e) { + assertEquals("failure1", e.getMessage()); + assertEquals(3L, e.getLimit()); + } + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java index a8266675..9d944f00 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameterTest.java @@ -41,7 +41,7 @@ public class MultipartRequestParameterTest { @Before public void before() { part1 = Mockito.mock(Part.class); - param = new MultipartRequestParameter(part1); + param = new MultipartRequestParameter(part1, null); } /** * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#setEncoding(java.lang.String)}. @@ -146,23 +146,36 @@ public void testGetSize() { * Test method for {@link org.apache.sling.engine.impl.parameters.MultipartRequestParameter#getString()}. */ @Test - public void testGetString() throws IOException { + public void testGetStringForFormField() throws IOException { mockPartInputStream(); - assertNotNull(param.getString()); + assertEquals("hi", param.getString()); // one more the for the cached value - assertNotNull(param.getString()); + assertEquals("hi", param.getString()); // with some invalid encoding param.setEncoding("invalid1"); - assertNotNull(param.getString()); + assertEquals("hi", param.getString()); // with some valid encoding param.setEncoding("UTF-8"); - assertNotNull(param.getString()); + assertEquals("hi", param.getString()); + } + + @Test + public void testGetStringForFileField() throws IOException { + mockPartInputStream(); // simulate an uploaded file field Mockito.doReturn("file1.txt").when(part1).getSubmittedFileName(); - assertNotNull(param.getString()); + assertEquals("hi", param.getString()); + + // with some invalid encoding + param.setEncoding("invalid1"); + assertEquals("", param.getString()); + + // with some valid encoding + param.setEncoding("UTF-8"); + assertEquals("hi", param.getString()); } /** diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java index 4e78c305..0583426e 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterMapTest.java @@ -18,7 +18,7 @@ */ package org.apache.sling.engine.impl.parameters; -import java.util.List; +import java.util.Collection; import java.util.Map; import jakarta.servlet.http.Part; @@ -198,10 +198,10 @@ public void testGetPart() { Part part2 = Mockito.mock(Part.class); Mockito.doReturn("key1").when(part2).getName(); - final MultipartRequestParameter param2 = new MultipartRequestParameter(part2); + final MultipartRequestParameter param2 = new MultipartRequestParameter(part2, null); map.addParameter(param2, true); - assertEquals(part2, map.getPart("key1")); + assertEquals("key1", ((Part) map.getPart("key1")).getName()); } /** @@ -220,10 +220,12 @@ public void testGetParts() { Part part2 = Mockito.mock(Part.class); Mockito.doReturn("key1").when(part2).getName(); - final MultipartRequestParameter param2 = new MultipartRequestParameter(part2); + final MultipartRequestParameter param2 = new MultipartRequestParameter(part2, null); map.setParameters("key1", new RequestParameter[] {param2}); - assertEquals(List.of(part2), map.getParts()); + Collection parts = map.getParts(); + assertEquals(1, parts.size()); + assertEquals("key1", ((Part) parts.iterator().next()).getName()); } /** diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java index c3a6ff41..d5395860 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java @@ -24,6 +24,7 @@ import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Enumeration; import java.util.HashMap; import java.util.Iterator; @@ -96,7 +97,10 @@ public void testGetParameterSupportRequestWrapper() { */ @Test(expected = None.class) public void testConfigure() { - ParameterSupport.configure(true); + ParameterSupport.configure(false, -1); + + // alternate options + ParameterSupport.configure(true, 50); } /** @@ -213,6 +217,28 @@ public void testMultipartPost() throws IOException, ServletException { assertEquals(4, list1.size()); } + @Test + public void testMultipartPostWithTooManyFiles() throws IOException, ServletException { + ParameterSupport.configure(true, 5); + + mockMultipartPost(); + + // add a few more parts + List parts = new ArrayList<>(mockRequest.getParts()); + for (int i = 0; i < 5; i++) { + Part mockPart = Mockito.mock(Part.class); + Mockito.doReturn(String.format("anotherfile%d.txt", i)) + .when(mockPart) + .getSubmittedFileName(); + parts.add(mockPart); + } + Mockito.doReturn(parts).when(mockRequest).getParts(); + + // expected FileCountLimitExceededException to be caught and logged but continue + final List list1 = support.getRequestParameterList(); + assertEquals(4, list1.size()); + } + @Test public void testStreamedMultipartPost1() throws IOException, ServletException { mockMultipartPost(); @@ -345,6 +371,7 @@ private void mockMultipartPost() throws IOException, ServletException { .getInputStream(); Part part2 = Mockito.mock(Part.class); Mockito.doReturn("key2").when(part2).getName(); + Mockito.doReturn("file2.txt").when(part2).getSubmittedFileName(); Mockito.doReturn(new ByteArrayInputStream("value2".getBytes())) .when(part2) .getInputStream(); diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java index 8da57a83..7c237f82 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.InputStream; +import java.util.ArrayList; import java.util.List; import java.util.NoSuchElementException; @@ -43,7 +44,7 @@ public class RequestPartsIteratorTest { @Before public void before() { mockRequest = Mockito.mock(HttpServletRequest.class); - iterator = new RequestPartsIterator(mockRequest); + iterator = new RequestPartsIterator(mockRequest, 5); } private Part mockParts() throws IOException, ServletException { @@ -76,6 +77,19 @@ public void testHasNextWithCaughtException() throws IOException, ServletExceptio assertFalse(iterator.hasNext()); } + @Test + public void testHasNextWithTooManyFiles() throws IOException, ServletException { + List parts = new ArrayList<>(); + for (int i = 0; i < 6; i++) { + Part mockPart = Mockito.mock(Part.class); + Mockito.doReturn(String.format("file%d.txt", i)).when(mockPart).getSubmittedFileName(); + parts.add(mockPart); + } + + Mockito.doReturn(parts).when(mockRequest).getParts(); + assertFalse(iterator.hasNext()); + } + /** * Test method for {@link org.apache.sling.engine.impl.parameters.RequestPartsIterator#next()}. */ diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/SlingPartTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/SlingPartTest.java new file mode 100644 index 00000000..0dd1ae4c --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/SlingPartTest.java @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.IOException; +import java.io.InputStream; +import java.util.List; + +import jakarta.servlet.http.Part; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.times; + +/** + * + */ +public class SlingPartTest { + + private Part part; + private SlingPart slingPart; + private MultipartRequestParameter mpr; + + @Before + public void before() { + part = Mockito.mock(Part.class); + mpr = new MultipartRequestParameter(part, null); + slingPart = new SlingPart(mpr); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getInputStream()}. + */ + @Test + public void testGetInputStream() throws IOException { + InputStream mockInputStream1 = Mockito.mock(InputStream.class); + Mockito.doReturn(mockInputStream1).when(part).getInputStream(); + assertEquals(mockInputStream1, slingPart.getInputStream()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getContentType()}. + */ + @Test + public void testGetContentType() { + String mockContentType = "text/plain"; + Mockito.doReturn(mockContentType).when(part).getContentType(); + assertEquals(mockContentType, slingPart.getContentType()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getName()}. + */ + @Test + public void testGetName() { + String mockName = "name1"; + Mockito.doReturn(mockName).when(part).getName(); + assertEquals(mockName, slingPart.getName()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getSize()}. + */ + @Test + public void testGetSize() { + long mockSize = 11L; + Mockito.doReturn(mockSize).when(part).getSize(); + assertEquals(mockSize, slingPart.getSize()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#write(java.lang.String)}. + */ + @Test + public void testWrite() { + assertThrows(IOException.class, () -> slingPart.write("file.txt")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#delete()}. + */ + @Test + public void testDelete() throws IOException { + slingPart.delete(); + Mockito.verify(part, times(1)).delete(); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getHeader(java.lang.String)}. + */ + @Test + public void testGetHeader() { + String mockHeaderValue = "value1"; + Mockito.doReturn(mockHeaderValue).when(part).getHeader("header1"); + assertEquals(mockHeaderValue, slingPart.getHeader("header1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getHeaders(java.lang.String)}. + */ + @Test + public void testGetHeaders() { + List mockHeaderValues = List.of("value1", "value2"); + Mockito.doReturn(mockHeaderValues).when(part).getHeaders("header1"); + assertEquals(mockHeaderValues, slingPart.getHeaders("header1")); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getHeaderNames()}. + */ + @Test + public void testGetHeaderNames() { + List mockHeaderNames = List.of("header1", "header2"); + Mockito.doReturn(mockHeaderNames).when(part).getHeaderNames(); + assertEquals(mockHeaderNames, slingPart.getHeaderNames()); + } + + /** + * Test method for {@link org.apache.sling.engine.impl.parameters.SlingPart#getSubmittedFileName()}. + */ + @Test + public void testGetSubmittedFileName() { + String mockFileName = "filename1.txt"; + Mockito.doReturn(mockFileName).when(part).getSubmittedFileName(); + assertEquals(mockFileName, slingPart.getSubmittedFileName()); + } +} From 5020c47f410b7f0d0e06e4238347cac260de1510 Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Wed, 20 May 2026 11:20:14 -0700 Subject: [PATCH 7/7] SLING-13202 pass the mulitpart maxFileCount value to the servlet config --- src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java index ca21f5f0..ee4caea2 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java @@ -200,6 +200,7 @@ private Dictionary getServletContextRegistrationProps(final Stri HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_MAXREQUESTSIZE, config.request_max()); servletConfig.put( HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_FILESIZETHRESHOLD, config.file_threshold()); + servletConfig.put("osgi.http.whiteboard.servlet.multipart.maxFileCount", config.request_max_file_count()); servletConfig.put(Constants.SERVICE_DESCRIPTION, "Apache Sling Engine Main Servlet"); servletConfig.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation");