From 6d21f56c18da43b3297eb51314c38e609ff5c555 Mon Sep 17 00:00:00 2001 From: Karan Kurani <85026744+kuranikaran@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:16:27 +0530 Subject: [PATCH 1/3] WW-5713 Fail closed for legacy Tiles OGNL evaluation --- plugins/tiles/README.md | 14 ++ .../tiles/DisabledOgnlAttributeEvaluator.java | 38 ++++ .../tiles/StrutsTilesContainerFactory.java | 34 +++- .../struts2/tiles/StrutsTilesInitializer.java | 12 +- .../struts2/tiles/StrutsTilesListener.java | 69 ++++++- .../apache/struts2/tiles/TilesConstants.java | 37 ++++ .../tiles/ognl/OGNLAttributeEvaluator.java | 5 + .../src/main/resources/struts-plugin.xml | 2 + .../DisabledOgnlAttributeEvaluatorTest.java | 44 +++++ .../StrutsTilesContainerFactoryTest.java | 85 +++++++- .../tiles/StrutsTilesListenerTest.java | 128 ++++++++++++ .../TilesOgnlEvaluatorIntegrationTest.java | 184 ++++++++++++++++++ 12 files changed, 644 insertions(+), 8 deletions(-) create mode 100644 plugins/tiles/src/main/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluator.java create mode 100644 plugins/tiles/src/main/java/org/apache/struts2/tiles/TilesConstants.java create mode 100644 plugins/tiles/src/test/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluatorTest.java create mode 100644 plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesListenerTest.java create mode 100644 plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java diff --git a/plugins/tiles/README.md b/plugins/tiles/README.md index 41a4fbe28b..a43c2624d9 100644 --- a/plugins/tiles/README.md +++ b/plugins/tiles/README.md @@ -4,3 +4,17 @@ You will find more details in [documentation](https://struts.apache.org/plugins/ ## Installation Just drop this plugin JAR into `WEB-INF/lib` folder or add it as a Maven dependency. + +## Legacy Tiles OGNL expressions + +The legacy Tiles `OGNL:` attribute-expression evaluator is deprecated in Struts 7.4.0 and disabled by default. Use +`S2:` for expressions that should be evaluated against the Struts ValueStack, or use an ordinary Tiles mechanism. + +Applications that temporarily require the legacy raw evaluator can set the following Struts constant: + +```xml + +``` + +Enabling the constant produces a startup warning. The compatibility constant is deprecated in Struts 7.4.0; both it +and the legacy evaluator are targeted for removal in Struts 8.0.0. diff --git a/plugins/tiles/src/main/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluator.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluator.java new file mode 100644 index 0000000000..775f6bdfaa --- /dev/null +++ b/plugins/tiles/src/main/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluator.java @@ -0,0 +1,38 @@ +/* + * 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.tiles; + +import org.apache.tiles.core.evaluator.AbstractAttributeEvaluator; +import org.apache.tiles.core.evaluator.EvaluationException; +import org.apache.tiles.request.Request; + +/** + * Fails closed when the deprecated Tiles OGNL evaluator has not been explicitly enabled. + */ +final class DisabledOgnlAttributeEvaluator extends AbstractAttributeEvaluator { + + static final String DISABLED_MESSAGE = "The Tiles OGNL evaluator is disabled. Migrate the expression to S2:, " + + "or temporarily enable struts.tiles.ognl.legacy.enabled. Legacy Tiles OGNL support will be removed in " + + "Struts 8.0.0."; + + @Override + public Object evaluate(String expression, Request request) { + throw new EvaluationException(DISABLED_MESSAGE, null); + } +} diff --git a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesContainerFactory.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesContainerFactory.java index 4111d1e7d5..20388c7018 100644 --- a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesContainerFactory.java +++ b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesContainerFactory.java @@ -39,6 +39,7 @@ import org.apache.tiles.core.definition.pattern.PrefixedPatternDefinitionResolver; import org.apache.tiles.core.definition.pattern.regexp.RegexpDefinitionPatternMatcherFactory; import org.apache.tiles.core.definition.pattern.wildcard.WildcardDefinitionPatternMatcherFactory; +import org.apache.tiles.core.evaluator.AttributeEvaluator; import org.apache.tiles.core.evaluator.AttributeEvaluatorFactory; import org.apache.tiles.core.evaluator.BasicAttributeEvaluatorFactory; import org.apache.tiles.core.evaluator.impl.DirectAttributeEvaluator; @@ -73,6 +74,7 @@ import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; /** * Dedicated Struts factory to build Tiles container with support for: @@ -89,6 +91,13 @@ public class StrutsTilesContainerFactory extends BasicTilesContainerFactory { private static final Logger LOG = LogManager.getLogger(StrutsTilesContainerFactory.class); + static final String LEGACY_OGNL_WARNING = "Legacy Tiles OGNL evaluation is enabled through " + + "struts.tiles.ognl.legacy.enabled. Migrate expressions to S2: or ordinary Tiles mechanisms; the " + + "compatibility flag and legacy evaluator will be removed in Struts 8.0.0."; + + private final boolean legacyOgnlEnabled; + private final AtomicBoolean legacyOgnlWarningLogged = new AtomicBoolean(); + /** * The freemarker renderer name. */ @@ -113,6 +122,14 @@ public class StrutsTilesContainerFactory extends BasicTilesContainerFactory { public static final String S2 = "S2"; public static final String I18N = "I18N"; + public StrutsTilesContainerFactory() { + this(false); + } + + StrutsTilesContainerFactory(boolean legacyOgnlEnabled) { + this.legacyOgnlEnabled = legacyOgnlEnabled; + } + @Override public TilesContainer createDecoratedContainer(TilesContainer originalContainer, ApplicationContext applicationContext) { return new CachingTilesContainer(originalContainer); @@ -155,7 +172,7 @@ protected AttributeEvaluatorFactory createAttributeEvaluatorFactory( BasicAttributeEvaluatorFactory attributeEvaluatorFactory = new BasicAttributeEvaluatorFactory(new DirectAttributeEvaluator()); attributeEvaluatorFactory.registerAttributeEvaluator(S2, createStrutsEvaluator()); attributeEvaluatorFactory.registerAttributeEvaluator(I18N, createI18NEvaluator()); - attributeEvaluatorFactory.registerAttributeEvaluator(OGNL, createOGNLEvaluator()); + attributeEvaluatorFactory.registerAttributeEvaluator(OGNL, createConfiguredOgnlEvaluator()); ELAttributeEvaluator elEvaluator = createELEvaluator(applicationContext); if (elEvaluator != null) { @@ -252,6 +269,21 @@ protected I18NAttributeEvaluator createI18NEvaluator() { return new I18NAttributeEvaluator(); } + private AttributeEvaluator createConfiguredOgnlEvaluator() { + if (legacyOgnlEnabled) { + if (legacyOgnlWarningLogged.compareAndSet(false, true)) { + logLegacyOgnlWarning(); + } + return createOGNLEvaluator(); + } + return new DisabledOgnlAttributeEvaluator(); + } + + void logLegacyOgnlWarning() { + LOG.warn(LEGACY_OGNL_WARNING); + } + + @SuppressWarnings("removal") protected OGNLAttributeEvaluator createOGNLEvaluator() { try { PropertyAccessor objectPropertyAccessor = OgnlRuntime.getPropertyAccessor(Object.class); diff --git a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesInitializer.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesInitializer.java index 03c1ebd8cf..12557ad946 100644 --- a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesInitializer.java +++ b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesInitializer.java @@ -32,6 +32,16 @@ public class StrutsTilesInitializer extends AbstractTilesInitializer { private static final Logger LOG = LogManager.getLogger(StrutsTilesInitializer.class); + private final boolean legacyOgnlEnabled; + + public StrutsTilesInitializer() { + this(false); + } + + StrutsTilesInitializer(boolean legacyOgnlEnabled) { + this.legacyOgnlEnabled = legacyOgnlEnabled; + } + @Override protected ApplicationContext createTilesApplicationContext(ApplicationContext preliminaryContext) { ServletContext servletContext = (ServletContext) preliminaryContext.getContext(); @@ -48,7 +58,7 @@ protected ApplicationContext createTilesApplicationContext(ApplicationContext pr @Override protected AbstractTilesContainerFactory createContainerFactory(ApplicationContext context) { LOG.trace("Creating dedicated Struts factory to create Tiles container"); - return new StrutsTilesContainerFactory(); + return new StrutsTilesContainerFactory(legacyOgnlEnabled); } } diff --git a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesListener.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesListener.java index 5ebe36237e..06a2c688b4 100644 --- a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesListener.java +++ b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesListener.java @@ -18,9 +18,15 @@ */ package org.apache.struts2.tiles; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletContextEvent; +import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.dispatcher.Dispatcher; +import org.apache.struts2.dispatcher.DispatcherListener; import org.apache.tiles.core.startup.TilesInitializer; +import org.apache.tiles.request.servlet.ServletApplicationContext; import org.apache.tiles.web.startup.AbstractTilesListener; /** @@ -28,13 +34,72 @@ * * @since Struts 2.0.2 */ -public class StrutsTilesListener extends AbstractTilesListener { +public class StrutsTilesListener extends AbstractTilesListener implements DispatcherListener { private static final Logger LOG = LogManager.getLogger(StrutsTilesListener.class); + private ServletContext servletContext; + private boolean legacyOgnlEnabled; + private boolean listeningToDispatcher; + + @Override + public void contextInitialized(ServletContextEvent event) { + servletContext = event.getServletContext(); + Dispatcher dispatcher = Dispatcher.getInstance(servletContext); + if (dispatcher == null) { + Dispatcher.addDispatcherListener(this); + listeningToDispatcher = true; + } else { + initializeTiles(dispatcher); + } + } + + @Override + public void contextDestroyed(ServletContextEvent event) { + if (listeningToDispatcher) { + Dispatcher.removeDispatcherListener(this); + listeningToDispatcher = false; + } + destroyTiles(); + servletContext = null; + } + + @Override + public void dispatcherInitialized(Dispatcher dispatcher) { + initializeTiles(dispatcher); + } + + @Override + public void dispatcherDestroyed(Dispatcher dispatcher) { + destroyTiles(); + } + @Override protected TilesInitializer createTilesInitializer() { LOG.info("Starting Struts Tiles 3 integration ..."); - return new StrutsTilesInitializer(); + return new StrutsTilesInitializer(legacyOgnlEnabled); + } + + boolean isLegacyOgnlEnabled() { + return legacyOgnlEnabled; + } + + @SuppressWarnings("removal") + private synchronized void initializeTiles(Dispatcher dispatcher) { + if (initializer != null) { + return; + } + String configuredValue = dispatcher.getContainer().getInstance( + String.class, TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED); + legacyOgnlEnabled = BooleanUtils.toBoolean(configuredValue); + initializer = createTilesInitializer(); + initializer.initialize(new ServletApplicationContext(servletContext)); + } + + private synchronized void destroyTiles() { + if (initializer != null) { + initializer.destroy(); + initializer = null; + } } } diff --git a/plugins/tiles/src/main/java/org/apache/struts2/tiles/TilesConstants.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/TilesConstants.java new file mode 100644 index 0000000000..44383047da --- /dev/null +++ b/plugins/tiles/src/main/java/org/apache/struts2/tiles/TilesConstants.java @@ -0,0 +1,37 @@ +/* + * 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.tiles; + +/** + * Constants used by the Tiles plugin. + */ +public final class TilesConstants { + + /** + * Temporarily enables legacy raw Tiles OGNL evaluation. + * + * @deprecated Migrate Tiles expressions to {@code S2:} or ordinary Tiles mechanisms. This compatibility + * constant and the legacy evaluator are targeted for removal in Struts 8.0.0. + */ + @Deprecated(since = "7.4.0", forRemoval = true) + public static final String STRUTS_TILES_OGNL_LEGACY_ENABLED = "struts.tiles.ognl.legacy.enabled"; + + private TilesConstants() { + } +} diff --git a/plugins/tiles/src/main/java/org/apache/tiles/ognl/OGNLAttributeEvaluator.java b/plugins/tiles/src/main/java/org/apache/tiles/ognl/OGNLAttributeEvaluator.java index cac54fdcb2..59aaff5223 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/ognl/OGNLAttributeEvaluator.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/ognl/OGNLAttributeEvaluator.java @@ -28,10 +28,15 @@ * Evaluates attribute expressions and expressions with OGNL language. * * @since 2.2.0 + * @deprecated This legacy evaluator does not use the Struts OGNL controls used by {@code S2:} and is disabled by + * default. Temporary use requires {@code struts.tiles.ognl.legacy.enabled=true}. Migrate to {@code S2:} or ordinary + * Tiles mechanisms. This evaluator is targeted for removal in Struts 8.0.0. */ +@Deprecated(since = "7.4.0", forRemoval = true) public class OGNLAttributeEvaluator extends AbstractAttributeEvaluator { /** {@inheritDoc} */ + @Override public Object evaluate(String expression, Request request) { if (expression == null) { throw new IllegalArgumentException("The expression parameter cannot be null"); diff --git a/plugins/tiles/src/main/resources/struts-plugin.xml b/plugins/tiles/src/main/resources/struts-plugin.xml index 09d33f5fa4..df2152900b 100644 --- a/plugins/tiles/src/main/resources/struts-plugin.xml +++ b/plugins/tiles/src/main/resources/struts-plugin.xml @@ -24,6 +24,8 @@ "https://struts.apache.org/dtds/struts-6.0.dtd"> + + diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluatorTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluatorTest.java new file mode 100644 index 0000000000..01e31f5804 --- /dev/null +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/DisabledOgnlAttributeEvaluatorTest.java @@ -0,0 +1,44 @@ +/* + * 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.tiles; + +import org.apache.tiles.api.Attribute; +import org.apache.tiles.api.Expression; +import org.apache.tiles.core.evaluator.EvaluationException; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; + +public class DisabledOgnlAttributeEvaluatorTest { + + @Test + public void failsClosedWithoutEvaluatingOrDisclosingExpression() { + String expression = "sensitive-marker.touch()"; + Attribute attribute = new Attribute(); + attribute.setExpressionObject(new Expression(expression)); + + EvaluationException exception = assertThrows(EvaluationException.class, + () -> new DisabledOgnlAttributeEvaluator().evaluate(attribute, null)); + + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, exception.getMessage()); + assertFalse(exception.getMessage().contains(expression)); + } +} diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesContainerFactoryTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesContainerFactoryTest.java index 89d4b53975..179ddd34d6 100644 --- a/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesContainerFactoryTest.java +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesContainerFactoryTest.java @@ -18,6 +18,9 @@ */ package org.apache.struts2.tiles; +import ognl.OgnlException; +import ognl.OgnlRuntime; +import ognl.PropertyAccessor; import org.apache.tiles.api.TilesContainer; import org.apache.tiles.core.evaluator.AttributeEvaluatorFactory; import org.apache.tiles.core.evaluator.impl.DirectAttributeEvaluator; @@ -42,11 +45,13 @@ import java.util.Objects; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@SuppressWarnings("removal") public class StrutsTilesContainerFactoryTest { private StrutsTilesContainerFactory factory; @@ -78,22 +83,64 @@ public void getSources() { } @Test - public void createAttributeEvaluatorFactory() { + public void createAttributeEvaluatorFactoryDefaultsToDisabledOgnlWithoutRawConstruction() { + TrackingFactory trackingFactory = new TrackingFactory(false); + PropertyAccessor requestAccessorBefore = getRequestAccessorOrNull(); LocaleResolver resolver = factory.createLocaleResolver(applicationContext); // explicitly disables support for EL JspFactory.setDefaultFactory(null); - AttributeEvaluatorFactory attributeEvaluatorFactory = factory.createAttributeEvaluatorFactory(applicationContext, resolver); + AttributeEvaluatorFactory attributeEvaluatorFactory = trackingFactory.createAttributeEvaluatorFactory(applicationContext, resolver); assertTrue("The class of the evaluator is not correct", attributeEvaluatorFactory.getAttributeEvaluator((String) null) instanceof DirectAttributeEvaluator); assertTrue("The class of the evaluator is not correct", attributeEvaluatorFactory.getAttributeEvaluator("S2") instanceof StrutsAttributeEvaluator); assertTrue("The class of the evaluator is not correct", - attributeEvaluatorFactory.getAttributeEvaluator("OGNL") instanceof OGNLAttributeEvaluator); + attributeEvaluatorFactory.getAttributeEvaluator("OGNL") instanceof DisabledOgnlAttributeEvaluator); assertTrue("The class of the evaluator is not correct", attributeEvaluatorFactory.getAttributeEvaluator("I18N") instanceof I18NAttributeEvaluator); assertTrue("The class of the evaluator is not correct", attributeEvaluatorFactory.getAttributeEvaluator("EL") instanceof DirectAttributeEvaluator); + assertEquals("The raw evaluator construction path must not run", 0, trackingFactory.rawEvaluatorCreations); + assertSame("The default path must not mutate the shared Tiles Request accessor", + requestAccessorBefore, getRequestAccessorOrNull()); + } + + @Test + public void createAttributeEvaluatorFactoryEnablesLegacyOgnlExplicitly() throws OgnlException { + PropertyAccessor originalAccessor = getRequestAccessorOrNull(); + try { + TrackingFactory trackingFactory = new TrackingFactory(true); + LocaleResolver resolver = trackingFactory.createLocaleResolver(applicationContext); + JspFactory.setDefaultFactory(null); + + AttributeEvaluatorFactory attributeEvaluatorFactory = trackingFactory.createAttributeEvaluatorFactory( + applicationContext, resolver); + + assertTrue(attributeEvaluatorFactory.getAttributeEvaluator("OGNL") instanceof OGNLAttributeEvaluator); + assertEquals(1, trackingFactory.rawEvaluatorCreations); + assertTrue(OgnlRuntime.getPropertyAccessor(org.apache.tiles.request.Request.class) + instanceof org.apache.tiles.ognl.DelegatePropertyAccessor); + } finally { + OgnlRuntime.setPropertyAccessor(org.apache.tiles.request.Request.class, originalAccessor); + } + } + + @Test + public void legacyWarningIsLoggedOncePerFactoryConstruction() throws OgnlException { + PropertyAccessor originalAccessor = getRequestAccessorOrNull(); + try { + TrackingFactory trackingFactory = new TrackingFactory(true); + JspFactory.setDefaultFactory(null); + LocaleResolver resolver = trackingFactory.createLocaleResolver(applicationContext); + trackingFactory.createAttributeEvaluatorFactory(applicationContext, resolver); + trackingFactory.createAttributeEvaluatorFactory(applicationContext, resolver); + + assertEquals(1, trackingFactory.legacyWarningCount); + assertEquals(StrutsTilesContainerFactory.LEGACY_OGNL_WARNING, trackingFactory.legacyWarningMessage); + } finally { + OgnlRuntime.setPropertyAccessor(org.apache.tiles.request.Request.class, originalAccessor); + } } @Test @@ -125,4 +172,34 @@ public void createDefaultAttributeRenderer() { verify(rendererFactory).getRenderer("freemarker"); } -} \ No newline at end of file + private static PropertyAccessor getRequestAccessorOrNull() { + try { + return OgnlRuntime.getPropertyAccessor(org.apache.tiles.request.Request.class); + } catch (OgnlException ignored) { + return null; + } + } + + private static class TrackingFactory extends StrutsTilesContainerFactory { + private int rawEvaluatorCreations; + private int legacyWarningCount; + private String legacyWarningMessage; + + private TrackingFactory(boolean legacyOgnlEnabled) { + super(legacyOgnlEnabled); + } + + @Override + protected OGNLAttributeEvaluator createOGNLEvaluator() { + rawEvaluatorCreations++; + return super.createOGNLEvaluator(); + } + + @Override + void logLegacyOgnlWarning() { + legacyWarningCount++; + legacyWarningMessage = LEGACY_OGNL_WARNING; + } + } + +} diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesListenerTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesListenerTest.java new file mode 100644 index 0000000000..d82ef4b769 --- /dev/null +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesListenerTest.java @@ -0,0 +1,128 @@ +/* + * 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.tiles; + +import jakarta.servlet.ServletContextEvent; +import org.apache.struts2.dispatcher.Dispatcher; +import org.apache.struts2.util.StrutsTestCaseHelper; +import org.apache.tiles.core.startup.TilesInitializer; +import org.apache.tiles.request.ApplicationContext; +import org.junit.Test; +import org.springframework.mock.web.MockServletContext; + +import java.util.Collections; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +@SuppressWarnings("removal") +public class StrutsTilesListenerTest { + + @Test + public void missingConfigurationUsesSecureDefault() { + assertConfiguredValue(Collections.emptyMap(), false); + } + + @Test + public void explicitFalseUsesSecureDefault() { + assertConfiguredValue(Map.of(TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED, "false"), false); + } + + @Test + public void explicitTrueEnablesLegacyMode() { + assertConfiguredValue(Map.of(TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED, "true"), true); + } + + @Test + public void mixedCaseTrueUsesNormalBooleanParsing() { + assertConfiguredValue(Map.of(TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED, "TrUe"), true); + } + + @Test + public void invalidValueUsesNormalFalseBooleanParsing() { + assertConfiguredValue(Map.of(TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED, "not-a-boolean"), false); + } + + @Test + public void dispatcherFirstUsesExistingLegacyConfigurationAndSingleLifecycle() { + MockServletContext servletContext = new MockServletContext(); + ServletContextEvent event = new ServletContextEvent(servletContext); + Dispatcher dispatcher = StrutsTestCaseHelper.initDispatcher(servletContext, + Map.of(TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED, "true")); + CapturingListener listener = new CapturingListener(); + try { + listener.contextInitialized(event); + + assertTrue(listener.legacyOgnlEnabled); + assertEquals(1, listener.initializations); + assertFalse("Tiles must remain active until servlet teardown", listener.destroyed); + } finally { + StrutsTestCaseHelper.tearDown(dispatcher); + listener.contextDestroyed(event); + } + assertTrue("Tiles must be destroyed", listener.destroyed); + assertEquals("Dispatcher and servlet teardown must destroy Tiles exactly once", 1, listener.destructions); + } + + private void assertConfiguredValue(Map constants, boolean expected) { + MockServletContext servletContext = new MockServletContext(); + ServletContextEvent event = new ServletContextEvent(servletContext); + CapturingListener listener = new CapturingListener(); + Dispatcher dispatcher = null; + listener.contextInitialized(event); + try { + dispatcher = StrutsTestCaseHelper.initDispatcher(servletContext, constants); + + assertEquals(expected, listener.legacyOgnlEnabled); + assertEquals(1, listener.initializations); + assertFalse("Tiles must remain active until dispatcher or servlet teardown", listener.destroyed); + } finally { + StrutsTestCaseHelper.tearDown(dispatcher); + listener.contextDestroyed(event); + } + assertTrue("Tiles must be destroyed exactly once", listener.destroyed); + assertEquals(1, listener.destructions); + } + + private static class CapturingListener extends StrutsTilesListener { + private boolean legacyOgnlEnabled; + private boolean destroyed; + private int initializations; + private int destructions; + + @Override + protected TilesInitializer createTilesInitializer() { + legacyOgnlEnabled = isLegacyOgnlEnabled(); + return new TilesInitializer() { + @Override + public void initialize(ApplicationContext preliminaryContext) { + initializations++; + } + + @Override + public void destroy() { + destroyed = true; + destructions++; + } + }; + } + } +} diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java new file mode 100644 index 0000000000..173ae0a33b --- /dev/null +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java @@ -0,0 +1,184 @@ +/* + * 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.tiles; + +import ognl.OgnlException; +import ognl.OgnlRuntime; +import ognl.PropertyAccessor; +import org.apache.struts2.ServletActionContext; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.Container; +import org.apache.struts2.util.StrutsTestCaseHelper; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; +import org.apache.tiles.api.Attribute; +import org.apache.tiles.api.Expression; +import org.apache.tiles.core.evaluator.AttributeEvaluatorFactory; +import org.apache.tiles.core.evaluator.EvaluationException; +import org.apache.tiles.core.impl.BasicTilesContainer; +import org.apache.tiles.request.ApplicationContext; +import org.apache.tiles.request.Request; +import org.apache.tiles.request.servlet.ServletApplicationContext; +import org.apache.tiles.request.servlet.ServletRequest; +import org.apache.tiles.request.render.BasicRendererFactory; +import org.apache.tiles.request.render.StringRenderer; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockServletContext; + +import jakarta.servlet.http.HttpServletResponse; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; + +/** + * Regression coverage for the two Tiles expression-language registrations. + */ +@SuppressWarnings("removal") +public class TilesOgnlEvaluatorIntegrationTest { + + private org.apache.struts2.dispatcher.Dispatcher dispatcher; + private Container strutsContainer; + private ValueStack valueStack; + private Request tilesRequest; + private BasicTilesContainer tilesContainer; + private BasicRendererFactory rendererFactory; + private StringWriter renderedOutput; + + @Before + public void setUp() throws Exception { + MockServletContext servletContext = new MockServletContext(); + dispatcher = StrutsTestCaseHelper.initDispatcher(servletContext, Map.of( + StrutsConstants.STRUTS_ALLOWLIST_ENABLE, Boolean.TRUE.toString() + )); + strutsContainer = dispatcher.getContainer(); + valueStack = strutsContainer.getInstance(ValueStackFactory.class).createValueStack(); + + MockHttpServletRequest servletRequest = new MockHttpServletRequest(servletContext); + servletRequest.setAttribute(ServletActionContext.STRUTS_VALUESTACK_KEY, valueStack); + ApplicationContext applicationContext = new ServletApplicationContext(servletContext); + HttpServletResponse servletResponse = mock(HttpServletResponse.class); + renderedOutput = new StringWriter(); + org.mockito.Mockito.when(servletResponse.getWriter()).thenReturn(new PrintWriter(renderedOutput)); + tilesRequest = new ServletRequest(applicationContext, servletRequest, servletResponse); + + StrutsTilesContainerFactory factory = new StrutsTilesContainerFactory(); + AttributeEvaluatorFactory evaluators = factory.createAttributeEvaluatorFactory( + applicationContext, factory.createLocaleResolver(applicationContext) + ); + rendererFactory = new BasicRendererFactory(); + rendererFactory.registerRenderer("string", new StringRenderer()); + tilesContainer = new BasicTilesContainer(); + tilesContainer.setAttributeEvaluatorFactory(evaluators); + tilesContainer.setRendererFactory(rendererFactory); + } + + @After + public void tearDown() { + StrutsTestCaseHelper.tearDown(dispatcher); + } + + @Test + public void ognlLanguageFailsClosedWithoutEvaluatingExpression() { + Marker marker = new Marker(); + tilesRequest.getContext("request").put("marker", marker); + + Attribute attribute = expression("marker.touch()", StrutsTilesContainerFactory.OGNL); + + EvaluationException exception = assertThrows(EvaluationException.class, + () -> tilesContainer.evaluate(attribute, tilesRequest)); + + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, exception.getMessage()); + assertFalse("The disabled evaluator must not invoke a method from the expression", marker.touched); + assertFalse("The exception must not disclose the expression", exception.getMessage().contains("marker.touch()")); + } + + @Test + public void legacyOgnlLanguagePreservesRawEvaluationWhenExplicitlyEnabled() throws OgnlException { + PropertyAccessor originalAccessor = getRequestAccessorOrNull(); + try { + Marker marker = new Marker(); + tilesRequest.getContext("request").put("marker", marker); + StrutsTilesContainerFactory legacyFactory = new StrutsTilesContainerFactory(true); + tilesContainer.setAttributeEvaluatorFactory(legacyFactory.createAttributeEvaluatorFactory( + tilesRequest.getApplicationContext(), legacyFactory.createLocaleResolver(tilesRequest.getApplicationContext()) + )); + + assertEquals("touched", tilesContainer.evaluate( + expression("marker.touch()", StrutsTilesContainerFactory.OGNL), tilesRequest)); + assertTrue("The explicitly enabled legacy evaluator must preserve existing behavior", marker.touched); + } finally { + OgnlRuntime.setPropertyAccessor(Request.class, originalAccessor); + } + } + + @Test + public void ordinaryDirectTilesAttributeRemainsUnaffected() throws Exception { + Attribute attribute = new Attribute("ordinary value"); + attribute.setRenderer("string"); + + assertEquals("ordinary value", tilesContainer.evaluate(attribute, tilesRequest)); + tilesContainer.render(attribute, tilesRequest); + assertEquals("ordinary value", renderedOutput.toString()); + } + + @Test + public void s2LanguageStillEvaluatesAgainstSecuredValueStack() { + Map model = new HashMap<>(); + model.put("title", "secured title"); + valueStack.push(model); + + assertEquals( + "secured title", + tilesContainer.evaluate(expression("title", StrutsTilesContainerFactory.S2), tilesRequest) + ); + } + + private static PropertyAccessor getRequestAccessorOrNull() { + try { + return OgnlRuntime.getPropertyAccessor(Request.class); + } catch (OgnlException ignored) { + return null; + } + } + + private Attribute expression(String value, String language) { + Attribute attribute = new Attribute(); + attribute.setExpressionObject(new Expression(value, language)); + return attribute; + } + + public static class Marker { + private boolean touched; + + public String touch() { + touched = true; + return "touched"; + } + } +} From f974b02441baa1b12ccfe5765124288c4b14bc9f Mon Sep 17 00:00:00 2001 From: Karan Kurani <85026744+kuranikaran@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:21:04 +0530 Subject: [PATCH 2/3] WW-5713 Isolate Tiles integration test JSP state --- .../tiles/TilesOgnlEvaluatorIntegrationTest.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java index 173ae0a33b..8d0e935dec 100644 --- a/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java @@ -45,6 +45,7 @@ import org.springframework.mock.web.MockServletContext; import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.jsp.JspFactory; import java.io.PrintWriter; import java.io.StringWriter; import java.util.HashMap; @@ -88,9 +89,16 @@ public void setUp() throws Exception { tilesRequest = new ServletRequest(applicationContext, servletRequest, servletResponse); StrutsTilesContainerFactory factory = new StrutsTilesContainerFactory(); - AttributeEvaluatorFactory evaluators = factory.createAttributeEvaluatorFactory( - applicationContext, factory.createLocaleResolver(applicationContext) - ); + JspFactory originalJspFactory = JspFactory.getDefaultFactory(); + AttributeEvaluatorFactory evaluators; + try { + JspFactory.setDefaultFactory(null); + evaluators = factory.createAttributeEvaluatorFactory( + applicationContext, factory.createLocaleResolver(applicationContext) + ); + } finally { + JspFactory.setDefaultFactory(originalJspFactory); + } rendererFactory = new BasicRendererFactory(); rendererFactory.registerRenderer("string", new StringRenderer()); tilesContainer = new BasicTilesContainer(); From 354f0da7e6acf4fb8ec5c4b5b44e5718605d1168 Mon Sep 17 00:00:00 2001 From: Karan Kurani <85026744+kuranikaran@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:19:03 +0530 Subject: [PATCH 3/3] WW-5713 Isolate all Tiles test factory construction --- .../TilesOgnlEvaluatorIntegrationTest.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java index 8d0e935dec..cccc116810 100644 --- a/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesOgnlEvaluatorIntegrationTest.java @@ -89,16 +89,7 @@ public void setUp() throws Exception { tilesRequest = new ServletRequest(applicationContext, servletRequest, servletResponse); StrutsTilesContainerFactory factory = new StrutsTilesContainerFactory(); - JspFactory originalJspFactory = JspFactory.getDefaultFactory(); - AttributeEvaluatorFactory evaluators; - try { - JspFactory.setDefaultFactory(null); - evaluators = factory.createAttributeEvaluatorFactory( - applicationContext, factory.createLocaleResolver(applicationContext) - ); - } finally { - JspFactory.setDefaultFactory(originalJspFactory); - } + AttributeEvaluatorFactory evaluators = createAttributeEvaluatorFactoryWithoutEl(factory, applicationContext); rendererFactory = new BasicRendererFactory(); rendererFactory.registerRenderer("string", new StringRenderer()); tilesContainer = new BasicTilesContainer(); @@ -133,9 +124,8 @@ public void legacyOgnlLanguagePreservesRawEvaluationWhenExplicitlyEnabled() thro Marker marker = new Marker(); tilesRequest.getContext("request").put("marker", marker); StrutsTilesContainerFactory legacyFactory = new StrutsTilesContainerFactory(true); - tilesContainer.setAttributeEvaluatorFactory(legacyFactory.createAttributeEvaluatorFactory( - tilesRequest.getApplicationContext(), legacyFactory.createLocaleResolver(tilesRequest.getApplicationContext()) - )); + tilesContainer.setAttributeEvaluatorFactory(createAttributeEvaluatorFactoryWithoutEl( + legacyFactory, tilesRequest.getApplicationContext())); assertEquals("touched", tilesContainer.evaluate( expression("marker.touch()", StrutsTilesContainerFactory.OGNL), tilesRequest)); @@ -175,6 +165,18 @@ private static PropertyAccessor getRequestAccessorOrNull() { } } + private static AttributeEvaluatorFactory createAttributeEvaluatorFactoryWithoutEl( + StrutsTilesContainerFactory factory, ApplicationContext applicationContext) { + JspFactory originalJspFactory = JspFactory.getDefaultFactory(); + try { + JspFactory.setDefaultFactory(null); + return factory.createAttributeEvaluatorFactory( + applicationContext, factory.createLocaleResolver(applicationContext)); + } finally { + JspFactory.setDefaultFactory(originalJspFactory); + } + } + private Attribute expression(String value, String language) { Attribute attribute = new Attribute(); attribute.setExpressionObject(new Expression(value, language));