diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 3436f7773b..7f4963593e 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -326,6 +326,37 @@ public final class StrutsConstants { public static final String STRUTS_FREEMARKER_WRAPPER_ALT_MAP = "struts.freemarker.wrapper.altMap"; + /** + * Controls FreeMarker whitespace stripping during template compilation. + * When enabled (default), removes indentation and trailing whitespace from lines containing only FTL tags. + * Automatically disabled when devMode is enabled. + * + * @since 7.2.0 + */ + public static final String STRUTS_FREEMARKER_WHITESPACE_STRIPPING = "struts.freemarker.whitespaceStripping"; + + /** + * Controls whether the compress tag is enabled globally. + * When disabled, the compress tag will not compress output regardless of other settings. + * + * @since 7.2.0 + */ + public static final String STRUTS_COMPRESS_ENABLED = "struts.compress.enabled"; + + /** + * Maximum size (in bytes) of body content that can be compressed. Content exceeding this limit will be skipped without compression. + * + * @since 7.2.0 + */ + public static final String STRUTS_COMPRESS_MAX_SIZE = "struts.compress.maxSize"; + + /** + * Maximum length of body content to include in log messages. Content longer than this will be truncated with length indicator. + * + * @since 7.2.0 + */ + public static final String STRUTS_COMPRESS_LOG_MAX_LENGTH = "struts.compress.log.maxLength"; + /** * Extension point for the Struts CompoundRootAccessor */ @@ -676,6 +707,7 @@ public final class StrutsConstants { /** * See {@link org.apache.struts2.interceptor.csp.CspNonceReader} + * * @since 6.8.0 */ public static final String STRUTS_CSP_NONCE_READER = "struts.csp.nonce.reader"; diff --git a/core/src/main/java/org/apache/struts2/components/Compress.java b/core/src/main/java/org/apache/struts2/components/Compress.java new file mode 100644 index 0000000000..34c2d50089 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/components/Compress.java @@ -0,0 +1,196 @@ +/* + * 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.struts2.components; + +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.Inject; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.views.annotations.StrutsTag; +import org.apache.struts2.views.annotations.StrutsTagAttribute; + +import java.io.Writer; + +/** + *

+ * Used to compress HTML output. Just wrap a given section with the tag. + *

+ * + *

+ * Security considerations: + *

+ * + * + *

+ * Configurable attributes are: + *

+ * + * + * + *

Examples

+ * 
+ *  
+ *  <s:compress>
+ *    <s:form action="submit">
+ *    <s:text name="name" />
+ *    ...
+ *    </s:form>
+ *  </s:compress>
+ *  
+ *
+ * + *

Uses conditional compression depending on action

+ * 
+ *  
+ *  <s:compress force="shouldCompress">
+ *    <s:form action="submit">
+ *    <s:text name="name" />
+ *    ...
+ *    </s:form>
+ *  </s:compress>
+ *  
+ *
+ * "shouldCompress" is a field with getter define on action used in expression evaluation + * + * @since 7.2.0 + */ +@StrutsTag(name = "compress", tldTagClass = "org.apache.struts2.views.jsp.CompressTag", + description = "Compress wrapped content\n\n

Security: The compress tag includes built-in protections against DoS attacks and sensitive data exposure. Large content exceeding the configured maximum size (default 10MB) will be skipped without compression. Log messages are automatically truncated to prevent sensitive data from appearing in logs.

") +public class Compress extends Component { + + private static final Logger LOG = LogManager.getLogger(Compress.class); + + private String force; + private boolean compressionEnabled = true; + private Long maxSize = null; + private int logMaxLength = 200; + + public Compress(ValueStack stack) { + super(stack); + } + + @Inject(value = StrutsConstants.STRUTS_COMPRESS_ENABLED, required = false) + public void setCompressionEnabled(String compressionEnabled) { + this.compressionEnabled = BooleanUtils.toBoolean(compressionEnabled); + } + + @Inject(value = StrutsConstants.STRUTS_COMPRESS_MAX_SIZE, required = false) + public void setMaxSize(String maxSize) { + try { + this.maxSize = Long.parseLong(maxSize.trim()); + } catch (NumberFormatException e) { + this.maxSize = null; + } + } + + @Inject(value = StrutsConstants.STRUTS_COMPRESS_LOG_MAX_LENGTH, required = false) + public void setLogMaxLength(String logMaxLength) { + try { + this.logMaxLength = Integer.parseInt(logMaxLength.trim()); + } catch (NumberFormatException e) { + this.logMaxLength = 200; + } + } + + @Override + public boolean end(Writer writer, String body) { + // Check size limit before processing + if (exceedsMaxSize(body) && compressionEnabled) { + LOG.warn("Body size: {} exceeds maximum allowed size: {}, skipping compression", body.length(), maxSize); + return super.end(writer, body, true); + } + + Object forceValue = findValue(force, Boolean.class); + + boolean forced = forceValue instanceof Boolean forcedValue && forcedValue; + + if (!compressionEnabled && !forced) { + if (LOG.isDebugEnabled()) { + LOG.debug("Compression disabled globally, skipping: {}", truncateForLogging(body)); + } + return super.end(writer, body, true); + } + if (devMode && !forced) { + if (LOG.isDebugEnabled()) { + LOG.debug("Avoids compressing output: {} in DevMode", truncateForLogging(body)); + } + return super.end(writer, body, true); + } + if (LOG.isTraceEnabled()) { + LOG.trace("Compresses: {}", truncateForLogging(body)); + } + String compressedBody = compressWhitespace(body); + if (LOG.isTraceEnabled()) { + LOG.trace("Compressed: {}", truncateForLogging(compressedBody)); + } + return super.end(writer, compressedBody, true); + } + + @Override + public boolean usesBody() { + return true; + } + + @StrutsTagAttribute(description = "Force output compression") + public void setForce(String force) { + this.force = force; + } + + private String truncateForLogging(String content) { + if (content == null) { + return null; + } + if (content.length() <= logMaxLength) { + return content; + } + return content.substring(0, logMaxLength) + "... (truncated, length: " + content.length() + ")"; + } + + private boolean exceedsMaxSize(String body) { + if (maxSize == null || body == null) { + return false; + } + return body.length() > maxSize; + } + + private String compressWhitespace(String input) { + if (input == null || input.isEmpty()) { + return input; + } + + // Early exit for very large inputs to prevent ReDoS and excessive processing + // This is a secondary check; primary size check happens in end() method + if (input.length() > 50_000_000) { // 50MB hard limit for regex operations + LOG.warn("Input size: {} exceeds safe processing limit (50MB), returning original content", + input.length()); + return input; + } + + // Simple compression: trim and remove whitespace between tags + return input.trim().replaceAll(">\\s+<", "><"); + } +} diff --git a/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java b/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java index ddc3682e6e..dcbebc767e 100644 --- a/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java +++ b/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java @@ -18,6 +18,7 @@ */ package org.apache.struts2.views.freemarker; +import org.apache.commons.lang3.BooleanUtils; import org.apache.struts2.FileManager; import org.apache.struts2.FileManagerFactory; import org.apache.struts2.inject.Container; @@ -111,23 +112,23 @@ public class FreemarkerManager { // copied from freemarker servlet - so that there is no dependency on it - public static final String INITPARAM_TEMPLATE_PATH = "TemplatePath"; - public static final String INITPARAM_NOCACHE = "NoCache"; - public static final String INITPARAM_CONTENT_TYPE = "ContentType"; - public static final String DEFAULT_CONTENT_TYPE = "text/html"; - public static final String INITPARAM_DEBUG = "Debug"; - - public static final String KEY_REQUEST = "Request"; - public static final String KEY_SESSION = "Session"; - public static final String KEY_APPLICATION = "Application"; - public static final String KEY_APPLICATION_PRIVATE = "__FreeMarkerServlet.Application__"; - public static final String KEY_JSP_TAGLIBS = "JspTaglibs"; - - // Note these names start with dot, so they're essentially invisible from a freemarker script. - private static final String ATTR_REQUEST_MODEL = ".freemarker.Request"; - private static final String ATTR_REQUEST_PARAMETERS_MODEL = ".freemarker.RequestParameters"; - private static final String ATTR_APPLICATION_MODEL = ".freemarker.Application"; - private static final String ATTR_JSP_TAGLIBS_MODEL = ".freemarker.JspTaglibs"; + public static final String INITPARAM_TEMPLATE_PATH = "TemplatePath"; + public static final String INITPARAM_NOCACHE = "NoCache"; + public static final String INITPARAM_CONTENT_TYPE = "ContentType"; + public static final String DEFAULT_CONTENT_TYPE = "text/html"; + public static final String INITPARAM_DEBUG = "Debug"; + + public static final String KEY_REQUEST = "Request"; + public static final String KEY_SESSION = "Session"; + public static final String KEY_APPLICATION = "Application"; + public static final String KEY_APPLICATION_PRIVATE = "__FreeMarkerServlet.Application__"; + public static final String KEY_JSP_TAGLIBS = "JspTaglibs"; + + // Note these names start with dot, so they're essentially invisible from a freemarker script. + private static final String ATTR_REQUEST_MODEL = ".freemarker.Request"; + private static final String ATTR_REQUEST_PARAMETERS_MODEL = ".freemarker.RequestParameters"; + private static final String ATTR_APPLICATION_MODEL = ".freemarker.Application"; + private static final String ATTR_JSP_TAGLIBS_MODEL = ".freemarker.JspTaglibs"; // for sitemesh public static final String ATTR_TEMPLATE_MODEL = ".freemarker.TemplateModel"; // template model stored in request for siteMesh @@ -162,7 +163,6 @@ public class FreemarkerManager { public static final String KEY_EXCEPTION = "exception"; - protected String templatePath; protected boolean nocache; protected boolean debug; @@ -176,7 +176,9 @@ public class FreemarkerManager { protected boolean cacheBeanWrapper; protected int mruMaxStrongSize; protected String templateUpdateDelay; - protected Map tagLibraries; + protected boolean whitespaceStripping = true; + protected boolean devMode; + protected Map tagLibraries; private FileManager fileManager; private FreemarkerThemeTemplateLoader themeTemplateLoader; @@ -203,7 +205,17 @@ public void setMruMaxStrongSize(String size) { @Inject(value = StrutsConstants.STRUTS_FREEMARKER_TEMPLATES_CACHE_UPDATE_DELAY, required = false) public void setTemplateUpdateDelay(String delay) { - templateUpdateDelay = delay; + templateUpdateDelay = delay; + } + + @Inject(value = StrutsConstants.STRUTS_FREEMARKER_WHITESPACE_STRIPPING, required = false) + public void setWhitespaceStripping(String whitespaceStripping) { + this.whitespaceStripping = BooleanUtils.toBoolean(whitespaceStripping); + } + + @Inject(value = StrutsConstants.STRUTS_DEVMODE, required = false) + public void setDevMode(String devMode) { + this.devMode = BooleanUtils.toBoolean(devMode); } @Inject @@ -295,7 +307,6 @@ public void init(ServletContext servletContext) throws TemplateException { * at the top. * * @param templateLoader the template loader - * * @see org.apache.struts2.views.freemarker.FreemarkerThemeTemplateLoader */ protected void configureTemplateLoader(TemplateLoader templateLoader) { @@ -341,8 +352,9 @@ protected Configuration createConfiguration(ServletContext servletContext) throw } LOG.debug("Disabled localized lookups"); configuration.setLocalizedLookup(false); - LOG.debug("Enabled whitespace stripping"); - configuration.setWhitespaceStripping(true); + boolean enableWhitespaceStripping = whitespaceStripping && !devMode; + LOG.debug("Whitespace stripping: {} (configured: {}, devMode: {})", enableWhitespaceStripping, whitespaceStripping, devMode); + configuration.setWhitespaceStripping(enableWhitespaceStripping); LOG.debug("Sets NewBuiltinClassResolver to TemplateClassResolver.SAFER_RESOLVER"); configuration.setNewBuiltinClassResolver(TemplateClassResolver.SAFER_RESOLVER); LOG.debug("Sets HTML as an output format and escaping policy"); @@ -413,7 +425,7 @@ protected ScopesHashModel buildScopesHashModel(ServletContext servletContext, Ht request.setAttribute(ATTR_REQUEST_PARAMETERS_MODEL, reqParametersModel); } model.put(ATTR_REQUEST_PARAMETERS_MODEL, reqParametersModel); - model.put(KEY_REQUEST_PARAMETERS_STRUTS,reqParametersModel); + model.put(KEY_REQUEST_PARAMETERS_STRUTS, reqParametersModel); return model; } @@ -426,56 +438,55 @@ protected ObjectWrapper createObjectWrapper(ServletContext servletContext) { } - /** + /** * Create the template loader. The default implementation will create a * {@link ClassTemplateLoader} if the template path starts with "class://", * a {@link FileTemplateLoader} if the template path starts with "file://", * and a {@link WebappTemplateLoader} otherwise. * * @param servletContext the servlet path - * @param templatePath the template path to create a loader for + * @param templatePath the template path to create a loader for * @return a newly created template loader */ protected TemplateLoader createTemplateLoader(ServletContext servletContext, String templatePath) { TemplateLoader templatePathLoader = null; - try { - if(templatePath!=null){ - if (templatePath.startsWith("class://")) { - // substring(7) is intentional as we "reuse" the last slash - templatePathLoader = new ClassTemplateLoader(getClass(), templatePath.substring(7)); - } else if (templatePath.startsWith("file://")) { - templatePathLoader = new FileTemplateLoader(new File(templatePath.substring(7))); - } - } - } catch (IOException e) { - LOG.error("Invalid template path specified: {}", e.getMessage(), e); - } - - // presume that most apps will require the class and webapp template loader - // if people wish to - return templatePathLoader != null ? - new MultiTemplateLoader(new TemplateLoader[]{ - templatePathLoader, - new WebappTemplateLoader(servletContext), - new StrutsClassTemplateLoader() - }) - : new MultiTemplateLoader(new TemplateLoader[]{ - new WebappTemplateLoader(servletContext), - new StrutsClassTemplateLoader() - }); - } + try { + if (templatePath != null) { + if (templatePath.startsWith("class://")) { + // substring(7) is intentional as we "reuse" the last slash + templatePathLoader = new ClassTemplateLoader(getClass(), templatePath.substring(7)); + } else if (templatePath.startsWith("file://")) { + templatePathLoader = new FileTemplateLoader(new File(templatePath.substring(7))); + } + } + } catch (IOException e) { + LOG.error("Invalid template path specified: {}", e.getMessage(), e); + } + + // presume that most apps will require the class and webapp template loader + // if people wish to + return templatePathLoader != null ? + new MultiTemplateLoader(new TemplateLoader[]{ + templatePathLoader, + new WebappTemplateLoader(servletContext), + new StrutsClassTemplateLoader() + }) + : new MultiTemplateLoader(new TemplateLoader[]{ + new WebappTemplateLoader(servletContext), + new StrutsClassTemplateLoader() + }); + } /** * Load the settings from the /freemarker.properties file on the classpath * * @param servletContext the servlet context - * * @see freemarker.template.Configuration#setSettings for the definition of valid settings */ protected void loadSettings(ServletContext servletContext) { - try (InputStream in = fileManager.loadFile(ClassLoaderUtil.getResource("freemarker.properties", getClass()))){ + try (InputStream in = fileManager.loadFile(ClassLoaderUtil.getResource("freemarker.properties", getClass()))) { if (in != null) { Properties p = new Properties(); p.load(in); @@ -528,7 +539,6 @@ public void addSetting(String name, String value) throws TemplateException { } - public ScopesHashModel buildTemplateModel(ValueStack stack, Object action, ServletContext servletContext, HttpServletRequest request, HttpServletResponse response, ObjectWrapper wrapper) { ScopesHashModel model = buildScopesHashModel(servletContext, request, response, wrapper, stack); populateContext(model, stack, action, request, response); diff --git a/core/src/main/java/org/apache/struts2/views/jsp/CompressTag.java b/core/src/main/java/org/apache/struts2/views/jsp/CompressTag.java new file mode 100644 index 0000000000..27d43a467d --- /dev/null +++ b/core/src/main/java/org/apache/struts2/views/jsp/CompressTag.java @@ -0,0 +1,56 @@ +/* + * 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.struts2.views.jsp; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.struts2.components.Component; +import org.apache.struts2.components.Compress; +import org.apache.struts2.util.ValueStack; + +import java.io.Serial; + +/** + * @see org.apache.struts2.components.Compress + * @since 7.2.0 + */ +public class CompressTag extends ComponentTagSupport { + + @Serial + private static final long serialVersionUID = 7572566991679717145L; + + private String force; + + @Override + public Component getBean(ValueStack stack, HttpServletRequest req, HttpServletResponse res) { + return new Compress(stack); + } + + @Override + protected void populateParams() { + super.populateParams(); + + Compress compress = (Compress) component; + compress.setForce(force); + } + + public void setForce(String force) { + this.force = force; + } +} diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 175530e0ef..283a5b1bca 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -207,6 +207,23 @@ struts.freemarker.wrapper.altMap=true ### check WW-3766 for more details struts.freemarker.mru.max.strong.size=0 +### Controls FreeMarker whitespace stripping during template compilation. +### Automatically disabled when devMode is enabled. +struts.freemarker.whitespaceStripping=true + +### Controls whether the compress tag is enabled globally. +struts.compress.enabled=true + +### Maximum size (in bytes) of body content that can be compressed. +### Content exceeding this limit will be skipped without compression. +### Default: 10MB (10485760 bytes) +struts.compress.maxSize=10485760 + +### Maximum length of body content to include in log messages. +### Content longer than this will be truncated with length indicator. +### Default: 200 characters +struts.compress.log.maxLength=200 + ### configure the XSLTResult class to use stylesheet caching. ### Set to true for developers and false for production. struts.xslt.nocache=false diff --git a/core/src/main/resources/template/css_xhtml/controlfooter.ftl b/core/src/main/resources/template/css_xhtml/controlfooter.ftl index d25b5790cf..0099b6df88 100644 --- a/core/src/main/resources/template/css_xhtml/controlfooter.ftl +++ b/core/src/main/resources/template/css_xhtml/controlfooter.ftl @@ -19,29 +19,26 @@ */ --> ${attributes.after!}<#t/> - <#lt/> <#if !attributes.labelPosition?? && (attributes.form.labelPosition)??> <#assign labelPos = attributes.form.labelPosition/> <#elseif attributes.labelPosition??> <#assign labelPos = attributes.labelPosition/> <#if (labelPos!"top") == 'top'> - <#rt/> +<#rt/> <#else> - <#rt/> +<#rt/> <#if (attributes.errorposition!"top") == 'bottom'> <#assign hasFieldErrors = attributes.name?? && fieldErrors?? && fieldErrors.get(attributes.name)??/> <#if hasFieldErrors>
<#if attributes.id??>id="wwerr_${attributes.id}"<#rt/> class="wwerr"> <#list fieldErrors.get(attributes.name) as error> - - <#if attributes.id??> - errorFor="${attributes.id}"<#rt/> - - class="errorMessage"> - ${error} -
<#t/> + +<#if attributes.id??> + errorFor="${attributes.id}"<#rt/> + + class="errorMessage">${error}<#rt/> <#t/> diff --git a/core/src/main/resources/template/css_xhtml/form-validate.ftl b/core/src/main/resources/template/css_xhtml/form-validate.ftl index 7beb7fab5f..1f1bff261f 100644 --- a/core/src/main/resources/template/css_xhtml/form-validate.ftl +++ b/core/src/main/resources/template/css_xhtml/form-validate.ftl @@ -20,9 +20,9 @@ --> <#if attributes.validate!false == true> <@s.script src="${base}${attributes.staticContentPath}/css_xhtml/validation.js"/> - <#if attributes.onsubmit??> - ${tag.addParameter('onsubmit', "${attributes.onsubmit}; return validateForm_${attributes.escapedId}();")} - <#else> - ${tag.addParameter('onsubmit', "return validateForm_${attributes.escapedId}();")} - +<#if attributes.onsubmit??> + ${tag.addParameter('onsubmit', "${attributes.onsubmit}; return validateForm_${attributes.escapedId}();")} +<#else> + ${tag.addParameter('onsubmit', "return validateForm_${attributes.escapedId}();")} + diff --git a/core/src/main/resources/template/css_xhtml/label.ftl b/core/src/main/resources/template/css_xhtml/label.ftl index 23701e7159..4c03cd19a5 100644 --- a/core/src/main/resources/template/css_xhtml/label.ftl +++ b/core/src/main/resources/template/css_xhtml/label.ftl @@ -18,7 +18,6 @@ * under the License. */ --> -<#--include "/${attributes.templateDir}/css_xhtml/controlheader.ftl" /--> <#include "/${attributes.templateDir}/${attributes.expandTheme}/controlheader.ftl" /> <#if attributes.id??> diff --git a/core/src/main/resources/template/simple/actionerror.ftl b/core/src/main/resources/template/simple/actionerror.ftl index 4cd9a60b93..54b3f3ca20 100644 --- a/core/src/main/resources/template/simple/actionerror.ftl +++ b/core/src/main/resources/template/simple/actionerror.ftl @@ -19,7 +19,7 @@ */ --> <#if (actionErrors?? && actionErrors?size > 0)> - + <#if attributes.id??> id="${attributes.id}"<#rt/> @@ -32,10 +32,10 @@ style="${attributes.cssStyle}"<#rt/> > - <#list actionErrors as error> - <#if error??> -
  • <#if attributes.escape>${error!}<#else>${error!?no_esc}<#rt/>
    • <#rt/> - - - +<#list actionErrors as error> +<#if error??> +
  • <#if attributes.escape>${error!}<#else>${error!?no_esc}<#rt/>
    • <#rt/> + + + \ No newline at end of file diff --git a/core/src/main/resources/template/simple/form-close-tooltips.ftl b/core/src/main/resources/template/simple/form-close-tooltips.ftl index 1bf45e909e..e57bdcc1e3 100644 --- a/core/src/main/resources/template/simple/form-close-tooltips.ftl +++ b/core/src/main/resources/template/simple/form-close-tooltips.ftl @@ -18,7 +18,6 @@ * under the License. */ --> - <#-- Code that will add javascript needed for tooltips --><#t/> diff --git a/core/src/main/resources/template/simple/form-close.ftl b/core/src/main/resources/template/simple/form-close.ftl index 59d4f0dbc2..291af09778 100644 --- a/core/src/main/resources/template/simple/form-close.ftl +++ b/core/src/main/resources/template/simple/form-close.ftl @@ -19,7 +19,6 @@ */ --> - <#if (attributes.customOnsubmitEnabled??)> <@s.script> <#-- @@ -98,5 +97,4 @@ - <#include "/${attributes.templateDir}/${attributes.expandTheme}/form-close-tooltips.ftl" /> diff --git a/core/src/main/resources/template/xhtml/controlheader.ftl b/core/src/main/resources/template/xhtml/controlheader.ftl index ec9372d927..2df6b1cb0d 100644 --- a/core/src/main/resources/template/xhtml/controlheader.ftl +++ b/core/src/main/resources/template/xhtml/controlheader.ftl @@ -19,10 +19,10 @@ */ --> <#include "/${attributes.templateDir}/${attributes.expandTheme}/controlheader-core.ftl" /> - - class="align-${attributes.align}" - <#else > - class="tdInput" - + +<#if attributes.align?? > + class="align-${attributes.align}"<#rt/> +<#else > + class="tdInput"<#rt/> + ><#t/> diff --git a/core/src/main/resources/template/xhtml/form-validate.ftl b/core/src/main/resources/template/xhtml/form-validate.ftl index a074bef2fd..cbd03a7f68 100644 --- a/core/src/main/resources/template/xhtml/form-validate.ftl +++ b/core/src/main/resources/template/xhtml/form-validate.ftl @@ -19,10 +19,10 @@ */ --> <#if attributes.validate!false == true> - <@s.script src="${base}${attributes.staticContentPath}/xhtml/validation.js" /> - <#if attributes.onsubmit??> - ${tag.addParameter('onsubmit', "${attributes.onsubmit}; return validateForm_${attributes.escapedId}();")} - <#else> - ${tag.addParameter('onsubmit', "return validateForm_${attributes.escapedId}();")} - +<@s.script src="${base}${attributes.staticContentPath}/xhtml/validation.js" /> +<#if attributes.onsubmit??> + ${tag.addParameter('onsubmit', "${attributes.onsubmit}; return validateForm_${attributes.escapedId}();")} +<#else> + ${tag.addParameter('onsubmit', "return validateForm_${attributes.escapedId}();")} + diff --git a/core/src/site/resources/tags/compress-attributes.html b/core/src/site/resources/tags/compress-attributes.html new file mode 100644 index 0000000000..9ecf24227c --- /dev/null +++ b/core/src/site/resources/tags/compress-attributes.html @@ -0,0 +1,32 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

    Dynamic Attributes Allowed:

    false

    Name

    Required

    Default

    Evaluated

    Type

    Description

    forcefalsefalseStringForce output compression
    performClearTagStateForTagPoolingServersfalsefalsefalseBooleanWhether to clear all tag state during doEndTag() processing (if applicable)
    diff --git a/core/src/site/resources/tags/compress-description.html b/core/src/site/resources/tags/compress-description.html new file mode 100644 index 0000000000..a25a7190b3 --- /dev/null +++ b/core/src/site/resources/tags/compress-description.html @@ -0,0 +1,3 @@ +Compress wrapped content + +

    Security: The compress tag includes built-in protections against DoS attacks and sensitive data exposure. Large content exceeding the configured maximum size (default 10MB) will be skipped without compression. Log messages are automatically truncated to prevent sensitive data from appearing in logs.

    diff --git a/core/src/test/java/org/apache/struts2/components/CompressTest.java b/core/src/test/java/org/apache/struts2/components/CompressTest.java new file mode 100644 index 0000000000..d6252afd9e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/components/CompressTest.java @@ -0,0 +1,390 @@ +/* + * 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.struts2.components; + +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.io.StringWriter; +import java.util.Map; + +import static org.junit.Assert.assertNotEquals; + +public class CompressTest extends StrutsInternalTestCase { + + private ValueStack stack; + private Map context; + private Compress compress; + + public void testCompressHtmlOutput() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.setForce("false"); + compress.end(writer, body); + + String expected = "File upload: result

    File upload: result

    "; + assertEquals(expected, writer.toString()); + } + + public void testCompressHtmlOutputSimpleAlgorithm() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + assertEquals("File upload: result

    File upload: result

    ", writer.toString()); + } + + public void testAvoidCompressingInDevModeHtmlOutput() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("true"); + compress.end(writer, body); + + assertEquals(body, writer.toString()); + } + + public void testCompressHtmlOutputEvenInDevMode() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("true"); + compress.setForce("true"); + compress.end(writer, body); + + assertEquals("File upload: result

    File upload: result

    ", writer.toString()); + } + + public void testCompressHtmlOutputEvenInDevModeAndForceIsExpression() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + this.context.put("shouldCompress", Boolean.TRUE); + + compress.setDevMode("true"); + compress.setForce("shouldCompress"); + + StringWriter writer = new StringWriter(); + compress.end(writer, body); + + String expected = "File upload: result

    File upload: result

    "; + assertEquals(expected, writer.toString()); + } + + public void testCompressionDisabledGlobally() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.setCompressionEnabled("false"); + compress.end(writer, body); + + assertEquals(body, writer.toString()); + } + + public void testCompressionDisabledGloballyButForced() { + String body = """ + + + File upload: result + + +

    File upload: result

    + + + """; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.setCompressionEnabled("false"); + compress.setForce("true"); + compress.end(writer, body); + + String expected = "File upload: result

    File upload: result

    "; + assertEquals(expected, writer.toString()); + } + + public void testContentWithCRLineBreaks() { + + String body = "\r\rTest\r\r"; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + String expected = "Test"; + assertEquals(expected, writer.toString()); + } + + public void testContentWithLFLineBreaks() { + String body = "\n\nTest\n\n"; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + String expected = "Test"; + assertEquals(expected, writer.toString()); + } + + public void testContentWithCRLFLineBreaks() { + String body = "\r\n\r\nTest\r\n\r\n"; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + String expected = "Test"; + assertEquals(expected, writer.toString()); + } + + public void testContentWithMixedLineBreaks() { + String body = "\r\n\nTest\r\r\n"; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + String expected = "Test"; + assertEquals(expected, writer.toString()); + } + + public void testEmptyBody() { + String body = ""; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + assertEquals("", writer.toString()); + } + + public void testWhitespaceOnlyBody() { + String body = " \n\n\n "; + + StringWriter writer = new StringWriter(); + + compress.setDevMode("false"); + compress.end(writer, body); + + assertEquals("", writer.toString()); // Leading/trailing whitespace removed + } + + public void testMaxSizeLimit() { + // Create body larger than default maxSize (10MB) + StringBuilder largeBody = new StringBuilder(); + largeBody.append(""); + largeBody.append("x".repeat(11_000_000)); + largeBody.append(""); + + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.setMaxSize("10485760"); // 10MB + + compress.end(writer, largeBody.toString()); + + // Should return original content without compression + assertEquals(largeBody.toString(), writer.toString()); + } + + public void testMaxSizeDisabled() { + // Create body with whitespace that will be compressed + StringBuilder largeBody = new StringBuilder(); + largeBody.append("\n\n"); + largeBody.append("

    Content with whitespace

    \n".repeat(1_000_000)); + largeBody.append("\n"); + + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + // Don't set maxSize - defaults to null (no limit) + + compress.end(writer, largeBody.toString()); + + // Should compress even large content when limit is disabled + assertNotEquals("Content should be compressed when limit is disabled", largeBody.toString(), writer.toString()); + } + + public void testLogTruncation() { + String longBody = "x".repeat(500); + + compress.setLogMaxLength("200"); + + // Test that processing doesn't throw exceptions with long content + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.end(writer, longBody); + + // Should process without errors + assertNotNull(writer.toString()); + } + + public void testVeryLargeInputSafety() { + // Create input larger than 50MB hard limit + StringBuilder hugeBody = new StringBuilder(); + hugeBody.append(""); + hugeBody.append("x".repeat(60_000_000)); + hugeBody.append(""); + + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + // Don't set maxSize - defaults to null (no config limit) + + compress.end(writer, hugeBody.toString()); + + // Should return original content due to hard limit in compressWhitespace + assertEquals(hugeBody.toString(), writer.toString()); + } + + public void testInvalidMaxSizeConfiguration() { + // Test non-numeric value - should disable limit and compress + String largeBodyStr = "\n\n" + + "

    Content with whitespace

    \n".repeat(1_000) + + "\n"; + + compress.setDevMode("false"); + compress.setMaxSize("invalid"); + + StringWriter writer = new StringWriter(); + compress.end(writer, largeBodyStr); + + // Should compress when limit is disabled (invalid value) + assertNotEquals("Content should be compressed when limit is disabled (invalid value)", largeBodyStr, writer.toString()); + } + + public void testValidMaxSizeConfiguration() { + // Test valid value - create body to exceed 5MB limit + StringBuilder hugeBody = new StringBuilder(); + hugeBody.append(""); + hugeBody.append("x".repeat(6_000_000)); + hugeBody.append(""); + + compress.setDevMode("false"); + compress.setMaxSize("5242880"); // 5MB + + StringWriter writer = new StringWriter(); + compress.end(writer, hugeBody.toString()); + + // Should skip compression for content exceeding 5MB + assertEquals(hugeBody.toString(), writer.toString()); + } + + public void testInvalidLogMaxLengthConfiguration() { + // Test negative value - should use default (200) and work normally + compress.setLogMaxLength("-1"); + + String body = "Test"; + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.end(writer, body); + + assertNotNull(writer.toString()); + assertEquals("Test", writer.toString()); + } + + public void testValidLogMaxLengthConfiguration() { + // Test valid value - should work normally + compress.setLogMaxLength("500"); + + String body = "Test"; + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.end(writer, body); + + assertNotNull(writer.toString()); + assertEquals("Test", writer.toString()); + } + + @Override + public void setUp() throws Exception { + super.setUp(); + stack = container.getInstance(ValueStackFactory.class).createValueStack(); + context = stack.getContext(); + compress = new Compress(stack); + stack.push(compress); + } +} diff --git a/core/src/test/java/org/apache/struts2/views/freemarker/FreemarkerManagerTest.java b/core/src/test/java/org/apache/struts2/views/freemarker/FreemarkerManagerTest.java index 86d3fa34cd..d30e4e43b8 100644 --- a/core/src/test/java/org/apache/struts2/views/freemarker/FreemarkerManagerTest.java +++ b/core/src/test/java/org/apache/struts2/views/freemarker/FreemarkerManagerTest.java @@ -113,6 +113,60 @@ public void testIncompatibleImprovementsByServletContext() throws Exception { // then assertEquals(Configuration.VERSION_2_3_32, manager.config.getIncompatibleImprovements()); } + + public void testWhitespaceStrippingEnabledByDefault() throws Exception { + // given + FreemarkerManager manager = new FreemarkerManager(); + container.inject(manager); + + // when + manager.init(servletContext); + + // then + assertTrue(manager.config.getWhitespaceStripping()); + } + + public void testWhitespaceStrippingDisabledViaConfiguration() throws Exception { + // given + FreemarkerManager manager = new FreemarkerManager(); + container.inject(manager); + manager.setWhitespaceStripping("false"); + manager.setDevMode("false"); + + // when + manager.init(servletContext); + + // then + assertFalse(manager.config.getWhitespaceStripping()); + } + + public void testWhitespaceStrippingDisabledInDevMode() throws Exception { + // given + FreemarkerManager manager = new FreemarkerManager(); + container.inject(manager); + manager.setWhitespaceStripping("true"); + manager.setDevMode("true"); + + // when + manager.init(servletContext); + + // then + assertFalse(manager.config.getWhitespaceStripping()); + } + + public void testWhitespaceStrippingEnabledWhenNotInDevMode() throws Exception { + // given + FreemarkerManager manager = new FreemarkerManager(); + container.inject(manager); + manager.setWhitespaceStripping("true"); + manager.setDevMode("false"); + + // when + manager.init(servletContext); + + // then + assertTrue(manager.config.getWhitespaceStripping()); + } } class DummyFreemarkerManager extends FreemarkerManager { diff --git a/core/src/test/java/org/apache/struts2/views/jsp/AbstractUITagTest.java b/core/src/test/java/org/apache/struts2/views/jsp/AbstractUITagTest.java index df1e4323d0..6252c4554f 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/AbstractUITagTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/AbstractUITagTest.java @@ -23,6 +23,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ServletActionContext; +import org.apache.struts2.StrutsConstants; import org.apache.struts2.dispatcher.mapper.ActionMapper; import org.apache.struts2.dispatcher.mapper.DefaultActionMapper; import org.apache.struts2.views.jsp.ui.AbstractUITag; @@ -295,4 +296,18 @@ public static String normalize(Object obj, boolean appendSpace) { return buffer.toString(); } + + protected void setDevMode(final boolean devMode) { + setStrutsConstant(Collections.singletonMap(StrutsConstants.STRUTS_DEVMODE, Boolean.toString(devMode))); + } + + /** + * Overwrite the Struts Constant and reload container + */ + @Override + protected void setStrutsConstant(final Map overwritePropeties) { + super.setStrutsConstant(overwritePropeties); + stack.getActionContext().withContainer(container); + } + } diff --git a/core/src/test/java/org/apache/struts2/views/jsp/CompressTagTest.java b/core/src/test/java/org/apache/struts2/views/jsp/CompressTagTest.java new file mode 100644 index 0000000000..abcb285063 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/views/jsp/CompressTagTest.java @@ -0,0 +1,90 @@ +/* + * 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.struts2.views.jsp; + +import org.apache.struts2.views.jsp.ui.StrutsBodyContent; + +public class CompressTagTest extends AbstractUITagTest { + + public void testNoCompression() throws Exception { + setDevMode(true); + + CompressTag tag = new CompressTag(); + tag.setPageContext(pageContext); + + StrutsBodyContent bc = new StrutsBodyContent(null); + bc.print(""" +
    +
    + """ + ); + + tag.doStartTag(); + tag.setBodyContent(bc); + tag.doEndTag(); + + assertEquals(""" +
    +
    + """.stripTrailing(), + this.writer.toString()); + } + + public void testForceCompression() throws Exception { + setDevMode(true); + + CompressTag tag = new CompressTag(); + tag.setPageContext(pageContext); + tag.setForce("true"); + + StrutsBodyContent bc = new StrutsBodyContent(null); + bc.print(""" +
    +
    + """ + ); + + tag.doStartTag(); + tag.setBodyContent(bc); + tag.doEndTag(); + + assertEquals("
    ", + this.writer.toString()); + } + + public void testDefaultCompression() throws Exception { + CompressTag tag = new CompressTag(); + tag.setPageContext(pageContext); + + StrutsBodyContent bc = new StrutsBodyContent(null); + bc.print(""" +
    +
    + """ + ); + + tag.doStartTag(); + tag.setBodyContent(bc); + tag.doEndTag(); + + assertEquals("
    ", + this.writer.toString()); + } + +} \ No newline at end of file diff --git a/core/src/test/java/org/apache/struts2/views/jsp/ui/DebugTagTest.java b/core/src/test/java/org/apache/struts2/views/jsp/ui/DebugTagTest.java index d1e89a099e..ffc706e0f9 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/ui/DebugTagTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/ui/DebugTagTest.java @@ -19,13 +19,9 @@ package org.apache.struts2.views.jsp.ui; import org.apache.commons.lang3.StringUtils; -import org.apache.struts2.StrutsConstants; import org.apache.struts2.dispatcher.PrepareOperations; import org.apache.struts2.views.jsp.AbstractUITagTest; -import java.util.Collections; -import java.util.Map; - /** * Test case for {@link org.apache.struts2.components.Debug}. */ @@ -203,16 +199,4 @@ public void testTagAttributeOverrideDevModeFalse_clearTagStateSet() throws Excep PrepareOperations.clearDevModeOverride(); // Clear DevMode override. Avoid ThreadLocal side-effects if test thread re-used. } - private void setDevMode(final boolean devMode) { - setStrutsConstant(Collections.singletonMap(StrutsConstants.STRUTS_DEVMODE, Boolean.toString(devMode))); - } - - /** - * Overwrite the Struts Constant and reload container - */ - @Override - protected void setStrutsConstant(final Map overwritePropeties) { - super.setStrutsConstant(overwritePropeties); - stack.getActionContext().withContainer(container); - } } diff --git a/thoughts/shared/research/2025-09-24-WW-5256-freemarker-whitespace-compression.md b/thoughts/shared/research/2025-09-24-WW-5256-freemarker-whitespace-compression.md new file mode 100644 index 0000000000..5f85d53bc9 --- /dev/null +++ b/thoughts/shared/research/2025-09-24-WW-5256-freemarker-whitespace-compression.md @@ -0,0 +1,172 @@ +--- +date: 2025-09-24T09:18:27+02:00 +topic: "WW-5256 FreeMarker Whitespace Compression Implementation Analysis" +tags: [research, codebase, freemarker, compression, whitespace, WW-5256, configuration] +status: complete +--- + +# Research: WW-5256 FreeMarker Whitespace Compression Implementation Analysis + +**Date**: 2025-09-24T09:18:27+02:00 + +## Research Question +Analyze JIRA issue WW-5256 and FreeMarker whitespace documentation to determine what changes are needed to fulfill the requirements, ensuring compression is optional and can be disabled in devMode or via configuration flag. + +## Summary +WW-5256 requests reducing HTML output size from FreeMarker templates. A comprehensive implementation already exists in a feature branch but hasn't been merged. The current main branch has basic FreeMarker whitespace stripping hardcoded as enabled. To fulfill the requirements, we need to: + +1. **Merge the existing compress tag implementation** from feature branch `feature/WW-5256-compress` +2. **Add configuration option** for FreeMarker whitespace stripping (currently hardcoded) +3. **Integrate DevMode awareness** for both features (partially exists in compress tag) + +## Detailed Findings + +### Current Implementation Status + +#### Main Branch (Production) +- **FreeMarker whitespace stripping**: Hardcoded as enabled in `FreemarkerManager.java:345` +- **No configuration control**: Cannot be disabled through Struts properties +- **No compress tag**: Dedicated compression functionality doesn't exist + +#### Feature Branch (`feature/WW-5256-compress`) +- **Dedicated compress tag**: Complete implementation with `` tag +- **DevMode integration**: Automatically disables compression in development mode +- **HTML compression algorithm**: Uses regex `body.trim().replaceAll(">\\s+<", "><")` +- **Force override**: `force` attribute allows overriding DevMode behavior + +### FreeMarker Documentation Analysis + +#### Available Compression Techniques +1. **Whitespace Stripping**: Removes indentation and trailing whitespace from lines with only FTL tags +2. **Compress Directive**: `<#compress>` removes excess whitespace from generated output +3. **Manual Control**: `<#t>`, `<#rt>`, `<#lt>`, `<#nt>` directives for fine-grained control + +#### Best Practices +- Compression should be configurable based on environment (development vs production) +- Multiple compression levels available for different use cases +- Template-level control allows fine-tuning specific outputs + +### Required Changes for Full Implementation + +#### 1. Configuration System Integration + +**New Configuration Constants** (add to `StrutsConstants.java`): +```java +public static final String STRUTS_FREEMARKER_WHITESPACE_STRIPPING = "struts.freemarker.whitespaceStripping"; +public static final String STRUTS_FREEMARKER_COMPRESSION_ENABLED = "struts.freemarker.compression.enabled"; +``` + +**Default Properties** (add to `default.properties`): +```properties +# FreeMarker whitespace stripping (currently hardcoded as true) +struts.freemarker.whitespaceStripping = true + +# FreeMarker HTML compression tag (new feature) +struts.freemarker.compression.enabled = true +``` + +#### 2. FreemarkerManager Modifications + +**Current Code** (`FreemarkerManager.java:345`): +```java +LOG.debug("Enabled whitespace stripping"); +configuration.setWhitespaceStripping(true); // Hardcoded! +``` + +**Required Change**: +```java +@Inject(value = StrutsConstants.STRUTS_FREEMARKER_WHITESPACE_STRIPPING, required = false) +public void setWhitespaceStripping(String whitespaceStripping) { + this.whitespaceStripping = BooleanUtils.toBoolean(whitespaceStripping); +} + +// In createConfiguration() method: +if (whitespaceStripping && !devMode) { + LOG.debug("Enabled whitespace stripping"); + configuration.setWhitespaceStripping(true); +} else { + LOG.debug("Disabled whitespace stripping (devMode: {})", devMode); + configuration.setWhitespaceStripping(false); +} +``` + +#### 3. Compress Component Enhancement + +**Merge from Feature Branch**: +- `core/src/main/java/org/apache/struts2/components/Compress.java` +- `core/src/main/java/org/apache/struts2/views/jsp/CompressTag.java` +- Test files: `CompressTest.java`, `CompressTagTest.java` +- Documentation files + +**Add Global Configuration Support**: +```java +@Inject(value = StrutsConstants.STRUTS_FREEMARKER_COMPRESSION_ENABLED, required = false) +public void setCompressionEnabled(String compressionEnabled) { + this.compressionEnabled = BooleanUtils.toBoolean(compressionEnabled); +} + +@Inject(StrutsConstants.STRUTS_DEVMODE) +public void setDevMode(String devMode) { + this.devMode = BooleanUtils.toBoolean(devMode); +} +``` + +## Code References + +### Current Implementation +- `core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java:345` - Hardcoded whitespace stripping +- `core/src/main/java/org/apache/struts2/StrutsConstants.java` - Configuration constants +- `core/src/main/resources/org/apache/struts2/default.properties` - Default configuration values + +### Feature Branch Implementation +- Commit: `a98ba7717d391e643b578501086d1b40f82d9ca4` - "WW-5256 Implements dedicated tag to compress output" +- `feature/WW-5256-compress` branch contains complete compress tag implementation + +### Configuration Patterns +- `core/src/main/java/org/apache/struts2/components/Component.java` - DevMode injection pattern +- `core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java` - DevMode auto-configuration + +## Architecture Insights + +### Established Configuration Patterns +1. **Dependency Injection**: Use `@Inject` with `StrutsConstants` for configuration +2. **Boolean Conversion**: Use `BooleanUtils.toBoolean()` for string-to-boolean conversion +3. **DevMode Integration**: Check `devMode` flag to automatically adjust behavior +4. **Optional Parameters**: Use `required = false` for optional configuration + +### Security Considerations +- DevMode awareness prevents unexpected behavior in development +- Simple regex-based compression algorithm avoids complex parsing vulnerabilities +- Configuration allows administrators to control feature availability + +### Performance Considerations +- Whitespace stripping happens during template compilation (minimal runtime cost) +- HTML compression happens during output generation (measurable runtime cost) +- Configuration allows disabling features when not needed + +## Implementation Strategy + +### Phase 1: Make Current Whitespace Stripping Configurable +1. Add configuration constant and default value +2. Modify `FreemarkerManager` to inject and use the configuration +3. Add DevMode awareness to disable stripping in development + +### Phase 2: Merge Compress Tag Implementation +1. Cherry-pick or merge the feature branch implementation +2. Add global configuration support to the compress component +3. Ensure proper integration with existing DevMode patterns + +### Phase 3: Testing and Documentation +1. Test both features with various configuration combinations +2. Update documentation for new configuration options +3. Verify DevMode behavior works as expected + +## Open Questions +1. Should whitespace stripping be automatically disabled in DevMode (like the compress tag) or require explicit configuration? +2. Should there be different compression levels or is the current binary on/off sufficient? +3. Should the compress tag respect the global whitespace stripping setting or remain independent? + +## Related Research +- WW-5256: Reduce size of generate html out of freemarker tag templates +- FreeMarker Documentation: Whitespace handling best practices +- Struts Configuration Patterns: DevMode and optional feature implementation \ No newline at end of file diff --git a/thoughts/shared/research/2025-11-27-compress-component-security-implementation-plan.md b/thoughts/shared/research/2025-11-27-compress-component-security-implementation-plan.md new file mode 100644 index 0000000000..880cf57065 --- /dev/null +++ b/thoughts/shared/research/2025-11-27-compress-component-security-implementation-plan.md @@ -0,0 +1,675 @@ +--- +date: 2025-01-27T00:00:00Z +topic: "Security Enhancements Implementation Plan for Compress Component" +tags: [implementation, security, compress, dos-prevention, logging-security, redos-mitigation] +status: planning +component: core/src/main/java/org/apache/struts2/components/Compress.java +related_issues: [security-review-compress-component] +--- + +# Implementation Plan: Security Enhancements for Compress Component + +**Date**: 2025-01-27 + +## Objective + +Implement security enhancements to address three identified security issues in the `Compress` component: +1. **Logging Sensitive Data** - Truncate body content in log messages +2. **DoS via Large Input** - Add configurable size limit +3. **ReDoS Mitigation** - Add performance safeguards for regex operations + +## Security Issues Summary + +### Issue 1: Logging Sensitive Data (Medium Risk) +- **Location**: `Compress.java:101, 105, 108, 111` +- **Problem**: Full body content logged at debug/trace level +- **Risk**: Sensitive data (passwords, tokens, PII) may be exposed in logs +- **Solution**: Implement truncation with length indicator + +### Issue 2: DoS via Large Input (Medium-High Risk) +- **Location**: `Compress.java:95-112` (no size check) +- **Problem**: No limit on body size, potential memory exhaustion +- **Risk**: OutOfMemoryError, CPU exhaustion, service disruption +- **Solution**: Add configurable maximum size check + +### Issue 3: ReDoS Potential (Low-Medium Risk) +- **Location**: `Compress.java:141-161` (regex operations) +- **Problem**: Regex patterns could be slow on malicious input +- **Risk**: CPU exhaustion from regex backtracking +- **Solution**: Add hard limit and optimize regex patterns + +## Implementation Phases + +### Phase 1: Configuration Constants + +**File**: `core/src/main/java/org/apache/struts2/StrutsConstants.java` +**Location**: After line 344 (after `STRUTS_COMPRESS_ENABLED`) + +**Changes**: +```java +/** + * Maximum size (in bytes) of body content that can be compressed. + * Content exceeding this limit will be skipped without compression. + * Set to 0 or negative to disable size checking (not recommended). + * Default: 10MB (10485760 bytes) + * + * @since 7.2.0 + */ +public static final String STRUTS_COMPRESS_MAX_SIZE = "struts.compress.maxSize"; + +/** + * Maximum length of body content to include in log messages. + * Content longer than this will be truncated with length indicator. + * Default: 200 characters + * + * @since 7.2.0 + */ +public static final String STRUTS_COMPRESS_LOG_MAX_LENGTH = "struts.compress.log.maxLength"; +``` + +**Validation**: +- [ ] Constants added with proper JavaDoc +- [ ] Follows existing naming conventions +- [ ] Proper `@since` annotation + +--- + +### Phase 2: Default Properties + +**File**: `core/src/main/resources/org/apache/struts2/default.properties` +**Location**: After line 215 (after `struts.compress.enabled=true`) + +**Changes**: +```properties +### Maximum size (in bytes) of body content that can be compressed. +### Content exceeding this limit will be skipped without compression. +### Default: 10MB (10485760 bytes) +struts.compress.maxSize=10485760 + +### Maximum length of body content to include in log messages. +### Content longer than this will be truncated with length indicator. +### Default: 200 characters +struts.compress.log.maxLength=200 +``` + +**Validation**: +- [ ] Properties added with descriptive comments +- [ ] Default values are reasonable (10MB, 200 chars) +- [ ] Follows existing property file format + +--- + +### Phase 3: Compress Component Updates + +**File**: `core/src/main/java/org/apache/struts2/components/Compress.java` + +#### 3.1: Add New Fields + +**Location**: After line 83 (after `private boolean compressionEnabled = true;`) + +**Changes**: +```java +private Long maxSize = null; +private int logMaxLength = 200; +``` + +#### 3.2: Add Injection Methods + +**Location**: After `setCompressionEnabled` method (after line 92) + +**Changes**: +```java +@Inject(value = StrutsConstants.STRUTS_COMPRESS_MAX_SIZE, required = false) +public void setMaxSize(String maxSize) { + if (maxSize != null && !maxSize.trim().isEmpty()) { + try { + this.maxSize = Long.parseLong(maxSize.trim()); + if (this.maxSize < 0) { + LOG.warn("Invalid maxSize value: {}, must be >= 0. Disabling size limit.", maxSize); + this.maxSize = null; + } + } catch (NumberFormatException e) { + LOG.warn("Invalid maxSize value: {}, must be a valid number. Disabling size limit.", maxSize, e); + this.maxSize = null; + } + } +} + +@Inject(value = StrutsConstants.STRUTS_COMPRESS_LOG_MAX_LENGTH, required = false) +public void setLogMaxLength(String logMaxLength) { + if (logMaxLength != null && !logMaxLength.trim().isEmpty()) { + try { + int length = Integer.parseInt(logMaxLength.trim()); + if (length < 0) { + LOG.warn("Invalid logMaxLength value: {}, must be >= 0. Using default: 200.", logMaxLength); + this.logMaxLength = 200; + } else { + this.logMaxLength = length; + } + } catch (NumberFormatException e) { + LOG.warn("Invalid logMaxLength value: {}, must be a valid number. Using default: 200.", logMaxLength, e); + this.logMaxLength = 200; + } + } +} +``` + +#### 3.3: Add Helper Methods + +**Location**: After `setSingleLine` method (after line 128) + +**Changes**: +```java +/** + * Truncates content for safe logging to prevent sensitive data exposure + * and excessive log file growth. + * + * @param content the content to truncate + * @return truncated content with length indicator if truncated, original content otherwise + */ +private String truncateForLogging(String content) { + if (content == null) { + return null; + } + if (content.length() <= logMaxLength) { + return content; + } + return content.substring(0, logMaxLength) + "... (truncated, length: " + content.length() + ")"; +} + +/** + * Checks if the body content exceeds the maximum allowed size. + * + * @param body the body content to check + * @return true if body exceeds maximum size, false otherwise + */ +private boolean exceedsMaxSize(String body) { + if (maxSize == null || body == null) { + return false; + } + return body.length() > maxSize; +} +``` + +#### 3.4: Update end() Method + +**Location**: Replace lines 95-112 + +**Changes**: +```java +@Override +public boolean end(Writer writer, String body) { + // Check size limit before processing + if (exceedsMaxSize(body) && compressionEnabled) { + LOG.warn("Body size: {} exceeds maximum allowed size: {}, skipping compression", + body.length(), maxSize); + return super.end(writer, body, true); + } + + Object forceValue = findValue(force, Boolean.class); + Object singleLineValue = findValue(singleLine, Boolean.class); + + boolean forced = forceValue != null && Boolean.parseBoolean(forceValue.toString()); + if (!compressionEnabled && !forced) { + LOG.debug("Compression disabled globally, skipping: {}", truncateForLogging(body)); + return super.end(writer, body, true); + } + if (devMode && !forced) { + LOG.debug("Avoids compressing output: {} in DevMode", truncateForLogging(body)); + return super.end(writer, body, true); + } + LOG.trace("Compresses: {}", truncateForLogging(body)); + boolean useSingleLine = singleLineValue instanceof Boolean single && single; + String compressedBody = compressWhitespace(body, useSingleLine); + LOG.trace("Compressed: {}", truncateForLogging(compressedBody)); + return super.end(writer, compressedBody, true); +} +``` + +#### 3.5: Update compressWhitespace() Method + +**Location**: Replace lines 130-161 + +**Changes**: +```java +/** + * Compresses whitespace in the input string. + * + *

    This method normalizes line breaks (CR, LF, CRLF) to LF and collapses + * consecutive whitespace characters according to the specified mode.

    + * + *

    Security note: This method includes safeguards against ReDoS attacks + * by using simple, bounded regex patterns and early exit for very large inputs.

    + * + * @param input the input string to compress + * @param singleLine if true, removes all line breaks and collapses to single spaces; + * if false, preserves line structure with single line breaks + * @return the compressed string with normalized whitespace + */ +private String compressWhitespace(String input, boolean singleLine) { + if (input == null || input.isEmpty()) { + return input; + } + + // Early exit for very large inputs to prevent ReDoS and excessive processing + // This is a secondary check; primary size check happens in end() method + if (input.length() > 50_000_000) { // 50MB hard limit for regex operations + LOG.warn("Input size {} exceeds safe processing limit (50MB), returning original content", + input.length()); + return input; + } + + // Normalize all line breaks to \n (handles \r\n, \r, \n) + // This pattern is safe: simple alternation with no quantifiers + String normalized = input.replaceAll("\\r\\n|\\r", "\n"); + + if (singleLine) { + // Remove all line breaks and collapse whitespace to single space + // Pattern is safe: simple character class with quantifier + String compressed = normalized.replaceAll("\\s+", " ").strip(); + // Simple string replace (not regex) - safe + return compressed.replace("> <", "><"); + } else { + // Preserve line breaks but collapse other whitespace + // Patterns are safe: bounded character classes with simple quantifiers + return normalized + .replaceAll("[ \\t]+", " ") // Collapse spaces/tabs to single space + .replaceAll("\\n+", "\n") // Collapse multiple newlines to single + .replaceAll(" *\\n *", "\n") // Remove spaces around newlines + .strip(); // Remove leading/trailing whitespace + } +} +``` + +#### 3.6: Update Class JavaDoc + +**Location**: Update lines 34-46 + +**Changes**: Add security considerations section: +```java +/** + *

    + * Used to compress HTML output. Just wrap a given section with the tag. + *

    + * + *

    + * Security considerations: + *

    + *
      + *
    • Body content is truncated in log messages to prevent sensitive data exposure
      • + *
    • Maximum size limit prevents DoS attacks via large inputs (configurable via struts.compress.maxSize)
      • + *
    • Regex operations include safeguards against ReDoS attacks
      • + *
    + * + *

    + * Configurable attributes are: + *

    + * ... +``` + +**Validation Checklist**: +- [ ] New fields added with appropriate types +- [ ] Injection methods handle null/empty/invalid values gracefully +- [ ] Helper methods are private and well-documented +- [ ] `end()` method includes size check before processing +- [ ] All log statements use `truncateForLogging()` +- [ ] `compressWhitespace()` includes hard limit check +- [ ] JavaDoc updated with security notes +- [ ] Code follows existing patterns and conventions + +--- + +### Phase 4: Test Implementation + +**File**: `core/src/test/java/org/apache/struts2/components/CompressTest.java` +**Location**: Add new test methods before `setUp()` method (before line 345) + +#### Test 1: Max Size Limit Enforcement + +```java +public void testMaxSizeLimit() { + Compress compress = new Compress(stack); + + // Create body larger than default maxSize (10MB) + StringBuilder largeBody = new StringBuilder(); + largeBody.append(""); + for (int i = 0; i < 11_000_000; i++) { // ~11MB + largeBody.append("x"); + } + largeBody.append(""); + + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.setMaxSize("10485760"); // 10MB + + compress.end(writer, largeBody.toString()); + + // Should return original content without compression + assertEquals(largeBody.toString(), writer.toString()); +} +``` + +#### Test 2: Max Size Disabled + +```java +public void testMaxSizeDisabled() { + Compress compress = new Compress(stack); + + StringBuilder largeBody = new StringBuilder(); + largeBody.append(""); + for (int i = 0; i < 11_000_000; i++) { + largeBody.append("x"); + } + largeBody.append(""); + + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.setMaxSize(null); // No limit + + compress.end(writer, largeBody.toString()); + + // Should compress even large content when limit is disabled + assertNotEquals(largeBody.toString(), writer.toString()); +} +``` + +#### Test 3: Log Truncation + +```java +public void testLogTruncation() { + Compress compress = new Compress(stack); + + StringBuilder longBody = new StringBuilder(); + for (int i = 0; i < 500; i++) { + longBody.append("x"); + } + + compress.setLogMaxLength("200"); + + // Test that processing doesn't throw exceptions with long content + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.end(writer, longBody.toString()); + + // Should process without errors + assertNotNull(writer.toString()); +} +``` + +#### Test 4: Very Large Input Safety + +```java +public void testVeryLargeInputSafety() { + Compress compress = new Compress(stack); + + // Create input larger than 50MB hard limit + StringBuilder hugeBody = new StringBuilder(); + hugeBody.append(""); + for (int i = 0; i < 60_000_000; i++) { // ~60MB + hugeBody.append("x"); + } + hugeBody.append(""); + + StringWriter writer = new StringWriter(); + compress.setDevMode("false"); + compress.setMaxSize(null); // No config limit + + compress.end(writer, hugeBody.toString()); + + // Should return original content due to hard limit in compressWhitespace + assertEquals(hugeBody.toString(), writer.toString()); +} +``` + +#### Test 5: Invalid Configuration Values + +```java +public void testInvalidMaxSizeConfiguration() { + Compress compress = new Compress(stack); + + // Test negative value + compress.setMaxSize("-1"); + assertNull(compress.maxSize); // Should be null after invalid input + + // Test non-numeric value + compress.setMaxSize("invalid"); + assertNull(compress.maxSize); // Should be null after invalid input + + // Test valid value + compress.setMaxSize("5242880"); // 5MB + assertEquals(Long.valueOf(5242880L), compress.maxSize); +} + +public void testInvalidLogMaxLengthConfiguration() { + Compress compress = new Compress(stack); + + // Test negative value + compress.setLogMaxLength("-1"); + assertEquals(200, compress.logMaxLength); // Should use default + + // Test non-numeric value + compress.setLogMaxLength("invalid"); + assertEquals(200, compress.logMaxLength); // Should use default + + // Test valid value + compress.setLogMaxLength("500"); + assertEquals(500, compress.logMaxLength); +} +``` + +**Note**: Tests 5 require access to private fields. Use reflection or make fields package-private for testing, or test via behavior rather than direct field access. + +**Validation Checklist**: +- [ ] All new test methods added +- [ ] Tests cover size limit enforcement +- [ ] Tests cover disabled size limit +- [ ] Tests cover log truncation behavior +- [ ] Tests cover very large input safety +- [ ] Tests cover invalid configuration handling +- [ ] All existing tests still pass +- [ ] Run: `mvn test -Dtest=CompressTest -DskipAssembly` + +--- + +### Phase 5: Documentation Updates + +#### 5.1: Site Documentation - Description + +**File**: `core/src/site/resources/tags/compress-description.html` + +**Changes**: Add security section: +```html +

    Security: The compress tag includes built-in protections against DoS attacks +and sensitive data exposure. Large content exceeding the configured maximum size +(default 10MB) will be skipped without compression. Log messages are automatically +truncated to prevent sensitive data from appearing in logs.

    +``` + +#### 5.2: Site Documentation - Attributes + +**File**: `core/src/site/resources/tags/compress-attributes.html` + +**Changes**: Add note about security features in existing attribute descriptions if needed. + +**Validation Checklist**: +- [ ] Security section added to description +- [ ] Documentation is clear and accurate +- [ ] Follows existing documentation style + +--- + +## Implementation Checklist + +### Pre-Implementation +- [ ] Review existing code patterns in similar components +- [ ] Verify configuration injection patterns +- [ ] Check test structure and conventions + +### Phase 1: Configuration Constants +- [ ] Add `STRUTS_COMPRESS_MAX_SIZE` constant +- [ ] Add `STRUTS_COMPRESS_LOG_MAX_LENGTH` constant +- [ ] Add proper JavaDoc documentation +- [ ] Verify naming conventions + +### Phase 2: Default Properties +- [ ] Add `struts.compress.maxSize=10485760` +- [ ] Add `struts.compress.log.maxLength=200` +- [ ] Add descriptive comments +- [ ] Verify property file format + +### Phase 3: Component Updates +- [ ] Add `maxSize` and `logMaxLength` fields +- [ ] Add `setMaxSize()` injection method +- [ ] Add `setLogMaxLength()` injection method +- [ ] Add `truncateForLogging()` helper method +- [ ] Add `exceedsMaxSize()` helper method +- [ ] Update `end()` method with size check +- [ ] Update all log statements to use truncation +- [ ] Update `compressWhitespace()` with hard limit +- [ ] Update class JavaDoc with security notes + +### Phase 4: Tests +- [ ] Add `testMaxSizeLimit()` test +- [ ] Add `testMaxSizeDisabled()` test +- [ ] Add `testLogTruncation()` test +- [ ] Add `testVeryLargeInputSafety()` test +- [ ] Add `testInvalidMaxSizeConfiguration()` test +- [ ] Add `testInvalidLogMaxLengthConfiguration()` test +- [ ] Run all CompressTest tests +- [ ] Run CompressTagTest tests +- [ ] Verify all tests pass + +### Phase 5: Documentation +- [ ] Update compress-description.html +- [ ] Update compress-attributes.html if needed +- [ ] Verify documentation accuracy + +### Post-Implementation +- [ ] Code review +- [ ] Security review +- [ ] Performance testing +- [ ] Integration testing +- [ ] Update CHANGELOG if applicable + +--- + +## Configuration Examples + +### Default Configuration (Recommended) +```properties +struts.compress.maxSize=10485760 +struts.compress.log.maxLength=200 +``` + +### Custom Size Limit (5MB) +```properties +struts.compress.maxSize=5242880 +``` + +### Disable Size Limit (Not Recommended) +```properties +struts.compress.maxSize=0 +``` + +### Increase Log Truncation Length +```properties +struts.compress.log.maxLength=500 +``` + +--- + +## Security Considerations + +### Default Values Rationale +- **10MB max size**: Reasonable for HTML content, prevents DoS while allowing legitimate use +- **200 char log truncation**: Prevents sensitive data exposure while maintaining useful debugging info +- **50MB hard limit**: Absolute safety limit for regex operations, prevents ReDoS + +### Backward Compatibility +- All changes are backward compatible +- Default behavior preserved when limits are disabled +- Existing code continues to work without changes + +### Error Handling +- Invalid configuration values log warnings and use safe defaults +- Size limit violations log warnings and skip compression (fail-safe) +- Hard limit violations return original content (fail-safe) + +--- + +## Code References + +### Files to Modify +- `core/src/main/java/org/apache/struts2/StrutsConstants.java:344-365` - Add constants +- `core/src/main/resources/org/apache/struts2/default.properties:215-220` - Add defaults +- `core/src/main/java/org/apache/struts2/components/Compress.java:83-161` - Main implementation +- `core/src/test/java/org/apache/struts2/components/CompressTest.java:345+` - Add tests +- `core/src/site/resources/tags/compress-description.html` - Update docs + +### Related Patterns +- Size limit pattern: `AbstractMultiPartRequest.java:234-249` - `exceedsMaxStringLength()` +- Configuration injection: `AbstractMultiPartRequest.java:133-168` - Size limit injection +- Logging patterns: Standard Log4j2 parameterized logging + +--- + +## Testing Strategy + +### Unit Tests +- Test size limit enforcement +- Test size limit disabled +- Test log truncation +- Test very large input safety +- Test invalid configuration values + +### Integration Tests +- Verify existing CompressTagTest still passes +- Test with various configuration combinations +- Test with real HTML content + +### Performance Tests +- Verify no significant performance regression +- Test with typical HTML sizes (1KB - 1MB) +- Test with edge cases (empty, very small, at limits) + +### Security Tests +- Test with very large inputs (>10MB, >50MB) +- Test with inputs containing sensitive data +- Verify log truncation works correctly +- Verify size limits are enforced + +--- + +## Risk Assessment + +### Implementation Risks +- **Low**: Changes are isolated to Compress component +- **Low**: Backward compatible by default +- **Low**: Follows existing Struts patterns + +### Security Risks Addressed +- **Medium-High → Low**: DoS via large input (mitigated) +- **Medium → Low**: Sensitive data in logs (mitigated) +- **Low-Medium → Low**: ReDoS potential (mitigated) + +--- + +## Open Questions + +1. Should the hard limit (50MB) be configurable or remain fixed? + - **Decision**: Fixed for security, configurable limit is separate +2. Should size limit violations throw exceptions or just skip compression? + - **Decision**: Skip compression (fail-safe approach) +3. Should log truncation be configurable per log level? + - **Decision**: Single configuration for simplicity + +--- + +## Notes + +- All changes follow existing Struts patterns and conventions +- Default values chosen to balance security and usability +- Implementation is fail-safe (errors don't break functionality) +- Comprehensive test coverage ensures reliability +- Documentation updated to inform users of security features +