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/4] 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/4] 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/4] 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)); From 917ce74064e3da39cf2fb9802df684a8864cfe19 Mon Sep 17 00:00:00 2001 From: Karan Kurani <85026744+kuranikaran@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:27:48 +0530 Subject: [PATCH 4/4] WW-5713 Resolve legacy Tiles OGNL per webapp --- plugins/tiles/README.md | 4 +- .../tiles/StrutsTilesContainerFactory.java | 62 +++++- .../struts2/tiles/StrutsTilesInitializer.java | 9 +- .../struts2/tiles/StrutsTilesListener.java | 69 +------ .../StrutsTilesContainerFactoryTest.java | 195 +++++++++++++++++- .../tiles/StrutsTilesListenerTest.java | 103 +-------- .../TilesOgnlEvaluatorIntegrationTest.java | 103 ++++++++- .../struts2/tiles/struts-tiles-legacy.xml | 28 +++ 8 files changed, 388 insertions(+), 185 deletions(-) create mode 100644 plugins/tiles/src/test/resources/org/apache/struts2/tiles/struts-tiles-legacy.xml diff --git a/plugins/tiles/README.md b/plugins/tiles/README.md index a43c2624d9..7c2a98f392 100644 --- a/plugins/tiles/README.md +++ b/plugins/tiles/README.md @@ -16,5 +16,7 @@ Applications that temporarily require the legacy raw evaluator can set the follo ``` -Enabling the constant produces a startup warning. The compatibility constant is deprecated in Struts 7.4.0; both it +The plugin resolves this constant from the current web application's Struts configuration on the first `OGNL:` +evaluation and caches the result for that evaluator lifecycle. Enabling the constant produces a one-time migration +warning when the legacy evaluator is first used. 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/StrutsTilesContainerFactory.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsTilesContainerFactory.java index 20388c7018..636a2cfab7 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 @@ -26,12 +26,15 @@ import jakarta.el.ListELResolver; import jakarta.el.MapELResolver; import jakarta.el.ResourceBundleELResolver; +import jakarta.servlet.ServletContext; import jakarta.servlet.jsp.JspFactory; import ognl.OgnlException; import ognl.OgnlRuntime; import ognl.PropertyAccessor; +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.tiles.api.TilesContainer; import org.apache.tiles.core.definition.DefinitionsFactory; import org.apache.tiles.core.definition.pattern.DefinitionPatternMatcherFactory; @@ -39,6 +42,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.AbstractAttributeEvaluator; import org.apache.tiles.core.evaluator.AttributeEvaluator; import org.apache.tiles.core.evaluator.AttributeEvaluatorFactory; import org.apache.tiles.core.evaluator.BasicAttributeEvaluatorFactory; @@ -67,6 +71,8 @@ import org.apache.tiles.request.render.BasicRendererFactory; import org.apache.tiles.request.render.ChainedDelegateRenderer; import org.apache.tiles.request.render.Renderer; +import org.apache.tiles.request.servlet.NotAServletEnvironmentException; +import org.apache.tiles.request.servlet.ServletUtil; import java.util.ArrayList; import java.util.Collection; @@ -95,7 +101,7 @@ public class StrutsTilesContainerFactory extends BasicTilesContainerFactory { + "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 Boolean legacyOgnlEnabled; private final AtomicBoolean legacyOgnlWarningLogged = new AtomicBoolean(); /** @@ -123,10 +129,10 @@ public class StrutsTilesContainerFactory extends BasicTilesContainerFactory { public static final String I18N = "I18N"; public StrutsTilesContainerFactory() { - this(false); + legacyOgnlEnabled = null; } - StrutsTilesContainerFactory(boolean legacyOgnlEnabled) { + public StrutsTilesContainerFactory(boolean legacyOgnlEnabled) { this.legacyOgnlEnabled = legacyOgnlEnabled; } @@ -270,7 +276,14 @@ protected I18NAttributeEvaluator createI18NEvaluator() { } private AttributeEvaluator createConfiguredOgnlEvaluator() { - if (legacyOgnlEnabled) { + if (legacyOgnlEnabled == null) { + return new ConfiguredOgnlAttributeEvaluator(); + } + return createOgnlEvaluator(legacyOgnlEnabled); + } + + private AttributeEvaluator createOgnlEvaluator(boolean enabled) { + if (enabled) { if (legacyOgnlWarningLogged.compareAndSet(false, true)) { logLegacyOgnlWarning(); } @@ -279,6 +292,23 @@ private AttributeEvaluator createConfiguredOgnlEvaluator() { return new DisabledOgnlAttributeEvaluator(); } + @SuppressWarnings("removal") + boolean isLegacyOgnlEnabled(Request request) { + try { + ServletContext servletContext = ServletUtil.getServletRequest(request) + .getRequest().getServletContext(); + Dispatcher dispatcher = Dispatcher.getInstance(servletContext); + if (dispatcher == null) { + return false; + } + String configuredValue = dispatcher.getConfigurationManager().getConfiguration().getContainer().getInstance( + String.class, TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED); + return BooleanUtils.toBoolean(configuredValue); + } catch (NotAServletEnvironmentException ignored) { + return false; + } + } + void logLegacyOgnlWarning() { LOG.warn(LEGACY_OGNL_WARNING); } @@ -302,4 +332,28 @@ protected OGNLAttributeEvaluator createOGNLEvaluator() { } } + private final class ConfiguredOgnlAttributeEvaluator extends AbstractAttributeEvaluator { + + private volatile AttributeEvaluator delegate; + + @Override + public Object evaluate(String expression, Request request) { + return getDelegate(request).evaluate(expression, request); + } + + private AttributeEvaluator getDelegate(Request request) { + AttributeEvaluator result = delegate; + if (result == null) { + synchronized (this) { + result = delegate; + if (result == null) { + result = createOgnlEvaluator(isLegacyOgnlEnabled(request)); + delegate = result; + } + } + } + return result; + } + } + } 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 12557ad946..53219c1112 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,13 +32,13 @@ public class StrutsTilesInitializer extends AbstractTilesInitializer { private static final Logger LOG = LogManager.getLogger(StrutsTilesInitializer.class); - private final boolean legacyOgnlEnabled; + private final Boolean legacyOgnlEnabled; public StrutsTilesInitializer() { - this(false); + legacyOgnlEnabled = null; } - StrutsTilesInitializer(boolean legacyOgnlEnabled) { + public StrutsTilesInitializer(boolean legacyOgnlEnabled) { this.legacyOgnlEnabled = legacyOgnlEnabled; } @@ -58,6 +58,9 @@ protected ApplicationContext createTilesApplicationContext(ApplicationContext pr @Override protected AbstractTilesContainerFactory createContainerFactory(ApplicationContext context) { LOG.trace("Creating dedicated Struts factory to create Tiles container"); + if (legacyOgnlEnabled == null) { + 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 06a2c688b4..5ebe36237e 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,15 +18,9 @@ */ 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; /** @@ -34,72 +28,13 @@ * * @since Struts 2.0.2 */ -public class StrutsTilesListener extends AbstractTilesListener implements DispatcherListener { +public class StrutsTilesListener extends AbstractTilesListener { 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(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; - } + return new StrutsTilesInitializer(); } } 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 179ddd34d6..aa92da82d5 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 @@ -21,8 +21,15 @@ import ognl.OgnlException; import ognl.OgnlRuntime; import ognl.PropertyAccessor; +import org.apache.struts2.StrutsStatics; +import org.apache.struts2.config.Configuration; +import org.apache.struts2.config.ConfigurationManager; +import org.apache.struts2.dispatcher.Dispatcher; +import org.apache.struts2.inject.Container; import org.apache.tiles.api.TilesContainer; +import org.apache.tiles.core.evaluator.AttributeEvaluator; import org.apache.tiles.core.evaluator.AttributeEvaluatorFactory; +import org.apache.tiles.core.evaluator.EvaluationException; import org.apache.tiles.core.evaluator.impl.DirectAttributeEvaluator; import org.apache.tiles.core.locale.LocaleResolver; import org.apache.tiles.core.prepare.factory.BasicPreparerFactory; @@ -31,21 +38,35 @@ import org.apache.tiles.request.ApplicationContext; import org.apache.tiles.request.ApplicationResource; import org.apache.tiles.request.locale.URLApplicationResource; +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.ChainedDelegateRenderer; import org.apache.tiles.request.render.Renderer; +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.ServletContext; +import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.jsp.JspFactory; +import java.lang.reflect.Modifier; import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -56,13 +77,20 @@ public class StrutsTilesContainerFactoryTest { private StrutsTilesContainerFactory factory; private ApplicationContext applicationContext; + private JspFactory originalJspFactory; @Before public void setUp() throws Exception { + originalJspFactory = JspFactory.getDefaultFactory(); applicationContext = mock(ApplicationContext.class); factory = new StrutsTilesContainerFactory(); } + @After + public void tearDown() { + JspFactory.setDefaultFactory(originalJspFactory); + } + @Test public void getSources() { ApplicationResource pathResource = new URLApplicationResource( @@ -83,8 +111,8 @@ public void getSources() { } @Test - public void createAttributeEvaluatorFactoryDefaultsToDisabledOgnlWithoutRawConstruction() { - TrackingFactory trackingFactory = new TrackingFactory(false); + public void createAttributeEvaluatorFactoryDefersOgnlConfigurationUntilEvaluation() { + TrackingFactory trackingFactory = new TrackingFactory(); PropertyAccessor requestAccessorBefore = getRequestAccessorOrNull(); LocaleResolver resolver = factory.createLocaleResolver(applicationContext); // explicitly disables support for EL @@ -95,15 +123,23 @@ public void createAttributeEvaluatorFactoryDefaultsToDisabledOgnlWithoutRawConst 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 DisabledOgnlAttributeEvaluator); + AttributeEvaluator ognlEvaluator = attributeEvaluatorFactory.getAttributeEvaluator("OGNL"); 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); + assertEquals("Configuration must not be resolved during construction", 0, trackingFactory.configurationResolutions); assertSame("The default path must not mutate the shared Tiles Request accessor", requestAccessorBefore, getRequestAccessorOrNull()); + + EvaluationException exception = assertThrows(EvaluationException.class, + () -> ognlEvaluator.evaluate("ignored", mock(org.apache.tiles.request.Request.class))); + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, exception.getMessage()); + assertEquals(1, trackingFactory.configurationResolutions); + assertEquals(0, trackingFactory.rawEvaluatorCreations); + assertSame("The disabled path must not mutate the shared Tiles Request accessor", + requestAccessorBefore, getRequestAccessorOrNull()); } @Test @@ -126,6 +162,104 @@ public void createAttributeEvaluatorFactoryEnablesLegacyOgnlExplicitly() throws } } + @Test + public void publicFalseConstructorSelectsDisabledEvaluatorWithoutRequestLookup() { + TrackingFactory trackingFactory = new TrackingFactory(false); + JspFactory.setDefaultFactory(null); + AttributeEvaluatorFactory evaluators = trackingFactory.createAttributeEvaluatorFactory( + applicationContext, trackingFactory.createLocaleResolver(applicationContext)); + + EvaluationException exception = assertThrows(EvaluationException.class, + () -> evaluators.getAttributeEvaluator("OGNL").evaluate( + "ignored", mock(org.apache.tiles.request.Request.class))); + + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, exception.getMessage()); + assertEquals(0, trackingFactory.configurationResolutions); + assertEquals(0, trackingFactory.rawEvaluatorCreations); + } + + @Test + public void lazyConfigurationIsResolvedOnceAndLegacyEvaluatorIsReused() throws OgnlException { + PropertyAccessor originalAccessor = getRequestAccessorOrNull(); + try { + TrackingFactory trackingFactory = new TrackingFactory(); + trackingFactory.configuredLegacyOgnlEnabled = true; + JspFactory.setDefaultFactory(null); + AttributeEvaluatorFactory evaluators = trackingFactory.createAttributeEvaluatorFactory( + applicationContext, trackingFactory.createLocaleResolver(applicationContext)); + AttributeEvaluator evaluator = evaluators.getAttributeEvaluator("OGNL"); + + assertEquals(0, trackingFactory.configurationResolutions); + assertEquals(0, trackingFactory.rawEvaluatorCreations); + assertEquals(1, evaluator.evaluate("1", mock(org.apache.tiles.request.Request.class))); + assertEquals(2, evaluator.evaluate("2", mock(org.apache.tiles.request.Request.class))); + + assertEquals(1, trackingFactory.configurationResolutions); + assertEquals(1, trackingFactory.rawEvaluatorCreations); + assertEquals(1, trackingFactory.legacyWarningCount); + } finally { + OgnlRuntime.setPropertyAccessor(org.apache.tiles.request.Request.class, originalAccessor); + } + } + + @Test + public void lazyConfigurationIsResolvedOnceUnderConcurrentFirstUse() throws Exception { + TrackingFactory trackingFactory = new TrackingFactory(); + trackingFactory.blockConfigurationResolution = true; + JspFactory.setDefaultFactory(null); + AttributeEvaluator evaluator = trackingFactory.createAttributeEvaluatorFactory( + applicationContext, trackingFactory.createLocaleResolver(applicationContext)) + .getAttributeEvaluator("OGNL"); + ExecutorService executor = Executors.newFixedThreadPool(4); + try { + Future first = executor.submit(() -> assertThrows(EvaluationException.class, + () -> evaluator.evaluate("ignored", mock(org.apache.tiles.request.Request.class)))); + assertTrue(trackingFactory.configurationResolutionEntered.await(10, TimeUnit.SECONDS)); + Future second = executor.submit(() -> assertThrows(EvaluationException.class, + () -> evaluator.evaluate("ignored", mock(org.apache.tiles.request.Request.class)))); + Future third = executor.submit(() -> assertThrows(EvaluationException.class, + () -> evaluator.evaluate("ignored", mock(org.apache.tiles.request.Request.class)))); + trackingFactory.continueConfigurationResolution.countDown(); + first.get(10, TimeUnit.SECONDS); + second.get(10, TimeUnit.SECONDS); + third.get(10, TimeUnit.SECONDS); + + assertEquals(1, trackingFactory.configurationResolutions); + assertEquals(0, trackingFactory.rawEvaluatorCreations); + } finally { + executor.shutdownNow(); + } + } + + @Test + public void requestScopedConfigurationUsesNormalBooleanParsing() { + assertResolvedConfiguration("false", false); + assertResolvedConfiguration("TrUe", true); + assertResolvedConfiguration("not-a-boolean", false); + } + + @Test + public void customInitializerBooleanConstructorsArePublic() throws NoSuchMethodException { + assertTrue(Modifier.isPublic(StrutsTilesContainerFactory.class.getConstructor(boolean.class).getModifiers())); + assertTrue(Modifier.isPublic(StrutsTilesInitializer.class.getConstructor(boolean.class).getModifiers())); + } + + @Test + public void noArgInitializerPreservesLazyWebApplicationConfiguration() throws OgnlException { + PropertyAccessor originalAccessor = getRequestAccessorOrNull(); + try { + StrutsTilesContainerFactory initializedFactory = new ExposedInitializer().createFactory(applicationContext); + JspFactory.setDefaultFactory(null); + AttributeEvaluator evaluator = initializedFactory.createAttributeEvaluatorFactory( + applicationContext, initializedFactory.createLocaleResolver(applicationContext)) + .getAttributeEvaluator("OGNL"); + + assertEquals(1, evaluator.evaluate("1", createRequestWithConfiguredValue("true"))); + } finally { + OgnlRuntime.setPropertyAccessor(org.apache.tiles.request.Request.class, originalAccessor); + } + } + @Test public void legacyWarningIsLoggedOncePerFactoryConstruction() throws OgnlException { PropertyAccessor originalAccessor = getRequestAccessorOrNull(); @@ -180,15 +314,68 @@ private static PropertyAccessor getRequestAccessorOrNull() { } } + private void assertResolvedConfiguration(String configuredValue, boolean expected) { + assertEquals(expected, factory.isLegacyOgnlEnabled(createRequestWithConfiguredValue(configuredValue))); + } + + private ServletRequest createRequestWithConfiguredValue(String configuredValue) { + MockServletContext servletContext = new MockServletContext(); + Dispatcher dispatcher = mock(Dispatcher.class); + ConfigurationManager configurationManager = mock(ConfigurationManager.class); + Configuration configuration = mock(Configuration.class); + Container container = mock(Container.class); + when(dispatcher.getConfigurationManager()).thenReturn(configurationManager); + when(configurationManager.getConfiguration()).thenReturn(configuration); + when(configuration.getContainer()).thenReturn(container); + when(container.getInstance(String.class, TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED)) + .thenReturn(configuredValue); + servletContext.setAttribute(StrutsStatics.SERVLET_DISPATCHER, dispatcher); + ServletApplicationContext servletApplicationContext = new ServletApplicationContext(servletContext); + return new ServletRequest(servletApplicationContext, + new MockHttpServletRequest(servletContext), mock(HttpServletResponse.class)); + } + + private static class ExposedInitializer extends StrutsTilesInitializer { + + private StrutsTilesContainerFactory createFactory(ApplicationContext context) { + return (StrutsTilesContainerFactory) createContainerFactory(context); + } + } + private static class TrackingFactory extends StrutsTilesContainerFactory { private int rawEvaluatorCreations; private int legacyWarningCount; private String legacyWarningMessage; + private final AtomicInteger resolutionCounter = new AtomicInteger(); + private final CountDownLatch configurationResolutionEntered = new CountDownLatch(1); + private final CountDownLatch continueConfigurationResolution = new CountDownLatch(1); + private boolean configuredLegacyOgnlEnabled; + private boolean blockConfigurationResolution; + private int configurationResolutions; + + private TrackingFactory() { + super(); + } private TrackingFactory(boolean legacyOgnlEnabled) { super(legacyOgnlEnabled); } + @Override + boolean isLegacyOgnlEnabled(org.apache.tiles.request.Request request) { + configurationResolutions = resolutionCounter.incrementAndGet(); + configurationResolutionEntered.countDown(); + if (blockConfigurationResolution) { + try { + assertTrue(continueConfigurationResolution.await(10, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError(e); + } + } + return configuredLegacyOgnlEnabled; + } + @Override protected OGNLAttributeEvaluator createOGNLEvaluator() { rawEvaluatorCreations++; 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 index d82ef4b769..2c52bcc04c 100644 --- a/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesListenerTest.java +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesListenerTest.java @@ -18,111 +18,14 @@ */ 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.apache.struts2.dispatcher.DispatcherListener; 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++; - } - }; - } + public void doesNotParticipateInDispatcherLifecycle() { + assertFalse(DispatcherListener.class.isAssignableFrom(StrutsTilesListener.class)); } } 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 cccc116810..4373a25e08 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 @@ -120,21 +120,89 @@ public void ognlLanguageFailsClosedWithoutEvaluatingExpression() { @Test public void legacyOgnlLanguagePreservesRawEvaluationWhenExplicitlyEnabled() throws OgnlException { PropertyAccessor originalAccessor = getRequestAccessorOrNull(); + org.apache.struts2.dispatcher.Dispatcher legacyDispatcher = null; try { + EvaluationException disabled = assertThrows(EvaluationException.class, () -> tilesContainer.evaluate( + expression("marker.touch()", StrutsTilesContainerFactory.OGNL), tilesRequest)); + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, disabled.getMessage()); + + MockServletContext legacyServletContext = new MockServletContext(); + legacyDispatcher = StrutsTestCaseHelper.initDispatcher(legacyServletContext, Map.of( + "config", "struts-default.xml,org/apache/struts2/tiles/struts-tiles-legacy.xml", + StrutsConstants.STRUTS_ALLOWLIST_ENABLE, Boolean.TRUE.toString())); + assertEquals(Boolean.TRUE.toString(), legacyDispatcher.getConfigurationManager().getConfiguration() + .getContainer().getInstance( + String.class, TilesConstants.STRUTS_TILES_OGNL_LEGACY_ENABLED)); + ApplicationContext legacyApplicationContext = new ServletApplicationContext(legacyServletContext); + MockHttpServletRequest legacyServletRequest = new MockHttpServletRequest(legacyServletContext); + Request legacyTilesRequest = new ServletRequest( + legacyApplicationContext, legacyServletRequest, mock(HttpServletResponse.class)); Marker marker = new Marker(); - tilesRequest.getContext("request").put("marker", marker); - StrutsTilesContainerFactory legacyFactory = new StrutsTilesContainerFactory(true); - tilesContainer.setAttributeEvaluatorFactory(createAttributeEvaluatorFactoryWithoutEl( - legacyFactory, tilesRequest.getApplicationContext())); + legacyTilesRequest.getContext("request").put("marker", marker); + TrackingFactory legacyFactory = new TrackingFactory(); + BasicTilesContainer legacyTilesContainer = new BasicTilesContainer(); + legacyTilesContainer.setAttributeEvaluatorFactory(createAttributeEvaluatorFactoryWithoutEl( + legacyFactory, legacyApplicationContext)); - assertEquals("touched", tilesContainer.evaluate( - expression("marker.touch()", StrutsTilesContainerFactory.OGNL), tilesRequest)); + assertEquals("The raw evaluator must not be built during Tiles construction", + 0, legacyFactory.rawEvaluatorCreations); + assertEquals("touched", legacyTilesContainer.evaluate( + expression("marker.touch()", StrutsTilesContainerFactory.OGNL), legacyTilesRequest)); + assertEquals("touched", legacyTilesContainer.evaluate( + expression("marker.touch()", StrutsTilesContainerFactory.OGNL), legacyTilesRequest)); assertTrue("The explicitly enabled legacy evaluator must preserve existing behavior", marker.touched); + assertEquals("The compatibility constant must be resolved once per evaluator", 1, + legacyFactory.configurationResolutions); + assertEquals("The raw evaluator must be constructed once per evaluator", 1, + legacyFactory.rawEvaluatorCreations); + assertEquals("The migration warning must be emitted once per evaluator lifecycle", 1, + legacyFactory.legacyWarnings); + + EvaluationException stillDisabled = assertThrows(EvaluationException.class, () -> tilesContainer.evaluate( + expression("marker.touch()", StrutsTilesContainerFactory.OGNL), tilesRequest)); + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, stillDisabled.getMessage()); } finally { + StrutsTestCaseHelper.tearDown(legacyDispatcher); OgnlRuntime.setPropertyAccessor(Request.class, originalAccessor); } } + @Test + public void missingDispatcherFailsClosedAndCachesTheDecision() { + MockServletContext servletContext = new MockServletContext(); + ApplicationContext applicationContext = new ServletApplicationContext(servletContext); + Request request = new ServletRequest( + applicationContext, new MockHttpServletRequest(servletContext), mock(HttpServletResponse.class)); + TrackingFactory factory = new TrackingFactory(); + AttributeEvaluatorFactory evaluators = createAttributeEvaluatorFactoryWithoutEl(factory, applicationContext); + + EvaluationException first = assertThrows(EvaluationException.class, + () -> evaluators.getAttributeEvaluator(StrutsTilesContainerFactory.OGNL).evaluate("ignored", request)); + EvaluationException second = assertThrows(EvaluationException.class, + () -> evaluators.getAttributeEvaluator(StrutsTilesContainerFactory.OGNL).evaluate("ignored", request)); + + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, first.getMessage()); + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, second.getMessage()); + assertEquals(1, factory.configurationResolutions); + assertEquals(0, factory.rawEvaluatorCreations); + } + + @Test + public void nonServletRequestFailsClosedWithoutExposingEnvironmentFailure() { + TrackingFactory factory = new TrackingFactory(); + AttributeEvaluatorFactory evaluators = createAttributeEvaluatorFactoryWithoutEl( + factory, tilesRequest.getApplicationContext()); + + EvaluationException exception = assertThrows(EvaluationException.class, + () -> evaluators.getAttributeEvaluator(StrutsTilesContainerFactory.OGNL) + .evaluate("sensitive-expression", mock(Request.class))); + + assertEquals(DisabledOgnlAttributeEvaluator.DISABLED_MESSAGE, exception.getMessage()); + assertFalse(exception.getMessage().contains("sensitive-expression")); + assertEquals(1, factory.configurationResolutions); + assertEquals(0, factory.rawEvaluatorCreations); + } + @Test public void ordinaryDirectTilesAttributeRemainsUnaffected() throws Exception { Attribute attribute = new Attribute("ordinary value"); @@ -191,4 +259,27 @@ public String touch() { return "touched"; } } + + private static class TrackingFactory extends StrutsTilesContainerFactory { + private int configurationResolutions; + private int rawEvaluatorCreations; + private int legacyWarnings; + + @Override + boolean isLegacyOgnlEnabled(Request request) { + configurationResolutions++; + return super.isLegacyOgnlEnabled(request); + } + + @Override + protected org.apache.tiles.ognl.OGNLAttributeEvaluator createOGNLEvaluator() { + rawEvaluatorCreations++; + return super.createOGNLEvaluator(); + } + + @Override + void logLegacyOgnlWarning() { + legacyWarnings++; + } + } } diff --git a/plugins/tiles/src/test/resources/org/apache/struts2/tiles/struts-tiles-legacy.xml b/plugins/tiles/src/test/resources/org/apache/struts2/tiles/struts-tiles-legacy.xml new file mode 100644 index 0000000000..a9af2d28cd --- /dev/null +++ b/plugins/tiles/src/test/resources/org/apache/struts2/tiles/struts-tiles-legacy.xml @@ -0,0 +1,28 @@ + + + + + + +