This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch feat/WW-5294-jsp-direct-access-warning in repository https://gitbox.apache.org/repos/asf/struts.git
commit 3ff827535506d1eff5f0307cb09fad2612bcc068 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Feb 6 08:42:10 2026 +0100 feat(security): WW-5294 add warning when JSP tags accessed directly Add security warning to TagUtils.getStack() that logs when JSP tags are rendered outside of action scope (direct JSP access). This helps developers identify potential security issues where JSPs are accessed directly without going through the Struts action flow. The warning message includes a link to the security documentation at https://struts.apache.org/security/#never-expose-jsp-files-directly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../org/apache/struts2/views/jsp/TagUtils.java | 10 +- .../apache/struts2/views/jsp/ActionTagTest.java | 82 ++++----- .../org/apache/struts2/views/jsp/TagUtilsTest.java | 189 +++++++++++++++++++++ 3 files changed, 240 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java b/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java index e8dc60c81..a3bf1e1dd 100644 --- a/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java +++ b/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java @@ -19,6 +19,7 @@ package org.apache.struts2.views.jsp; import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionInvocation; import org.apache.struts2.config.ConfigurationException; import org.apache.struts2.util.ValueStack; import org.apache.logging.log4j.LogManager; @@ -44,7 +45,7 @@ public class TagUtils { if (stack == null) { LOG.warn("No ValueStack in ActionContext!"); throw new ConfigurationException("Rendering tag out of Action scope, accessing directly JSPs is not recommended! " + - "Please read https://struts.apache.org/security/#never-expose-jsp-files-directly";); + "Please read https://struts.apache.org/security/#never-expose-jsp-files-directly";); } else { LOG.trace("Adds the current PageContext to ActionContext"); stack.getActionContext() @@ -52,6 +53,13 @@ public class TagUtils { .with(ATTRIBUTES, new AttributeMap(stack.getContext())); } + // Check for direct JSP access (stack exists but no action invocation) + ActionInvocation ai = ActionContext.getContext().getActionInvocation(); + if (ai == null || ai.getAction() == null) { + LOG.warn("Rendering tag out of Action scope, accessing directly JSPs is not recommended! " + + "Please read https://struts.apache.org/security/#never-expose-jsp-files-directly";); + } + return stack; } diff --git a/core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java b/core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java index 3b60dca09..185349404 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java @@ -71,8 +71,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testActionTagWithNamespace_clearTagStateSet() { @@ -105,8 +105,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testSimple() { @@ -144,8 +144,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -187,8 +187,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -233,8 +233,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -276,8 +276,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -305,8 +305,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -337,8 +337,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -366,8 +366,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testActionWithoutExecuteResult_clearTagStateSet() throws Exception { @@ -397,13 +397,14 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testExecuteButResetReturnSameInvocation() throws Exception { Mock mockActionInv = new Mock(ActionInvocation.class); mockActionInv.matchAndReturn("invoke", "TEST"); + mockActionInv.matchAndReturn("getAction", new Object()); ActionTag tag = new ActionTag(); tag.setPageContext(pageContext); tag.setNamespace(""); @@ -426,14 +427,15 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testExecuteButResetReturnSameInvocation_clearTagStateSet() throws Exception { Mock mockActionInv = new Mock(ActionInvocation.class); mockActionInv.matchAndReturn("invoke", "TEST"); + mockActionInv.matchAndReturn("getAction", new Object()); ActionTag tag = new ActionTag(); tag.setPerformClearTagStateForTagPoolingServers(true); // Explicitly request tag state clearing. tag.setPageContext(pageContext); @@ -459,8 +461,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testIngoreContextParamsFalse() throws Exception { @@ -491,8 +493,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -527,8 +529,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -560,8 +562,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testIngoreContextParamsTrue_clearTagStateSet() throws Exception { @@ -595,8 +597,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testNoNameDefined() throws Exception { @@ -633,8 +635,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } // FIXME: Logging the error seems to cause the standard Maven build to fail @@ -656,8 +658,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testActionMethodWithExecuteResult() throws Exception { @@ -685,8 +687,8 @@ public class ActionTagTest extends AbstractTagTest { ActionTag freshTag = new ActionTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testActionMethodWithExecuteResult_clearTagStateSet() throws Exception { @@ -717,8 +719,8 @@ public class ActionTagTest extends AbstractTagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", - strutsBodyTagsAreReflectionEqual(tag, freshTag)); + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @Override diff --git a/core/src/test/java/org/apache/struts2/views/jsp/TagUtilsTest.java b/core/src/test/java/org/apache/struts2/views/jsp/TagUtilsTest.java new file mode 100644 index 000000000..0c997e27a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/views/jsp/TagUtilsTest.java @@ -0,0 +1,189 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.views.jsp; + +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.struts2.ActionContext; +import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.mock.MockActionInvocation; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.ServletActionContext; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.TestAction; + +import java.util.ArrayList; +import java.util.List; + +/** + * Tests for {@link TagUtils} class. + * Verifies security warning behavior when JSP tags are accessed directly without action flow. + */ +public class TagUtilsTest extends StrutsInternalTestCase { + + private StrutsMockHttpServletRequest request; + private StrutsMockPageContext pageContext; + private StrutsMockHttpServletResponse response; + private TestAppender testAppender; + private Logger tagUtilsLogger; + + @Override + protected void setUp() throws Exception { + super.setUp(); + + request = new StrutsMockHttpServletRequest(); + response = new StrutsMockHttpServletResponse(); + request.setSession(new StrutsMockHttpSession()); + + pageContext = new StrutsMockPageContext(servletContext, request, response); + + // Setup log appender to capture warnings + testAppender = new TestAppender(); + tagUtilsLogger = (Logger) LogManager.getLogger(TagUtils.class); + tagUtilsLogger.addAppender(testAppender); + testAppender.start(); + } + + @Override + protected void tearDown() throws Exception { + testAppender.stop(); + tagUtilsLogger.removeAppender(testAppender); + ActionContext.clear(); + super.tearDown(); + } + + public void testGetStack_withNullValueStack_throwsConfigurationException() { + // Setup: no ValueStack in request or ActionContext + ActionContext.of().bind(); + + try { + TagUtils.getStack(pageContext); + fail("Expected ConfigurationException to be thrown"); + } catch (ConfigurationException e) { + assertTrue("Exception message should contain 'Rendering tag out of Action scope'", + e.getMessage().contains("Rendering tag out of Action scope")); + assertTrue("Exception message should contain security warning", + e.getMessage().contains("accessing directly JSPs is not recommended")); + } + } + + public void testGetStack_withNullActionInvocation_logsWarning() { + // Setup: ValueStack exists but no ActionInvocation + ValueStack stack = ActionContext.getContext().getValueStack(); + request.setAttribute(ServletActionContext.STRUTS_VALUESTACK_KEY, stack); + + // Ensure ActionInvocation is null + ActionContext.of(stack.getContext()) + .withActionInvocation(null) + .bind(); + + // Execute + ValueStack result = TagUtils.getStack(pageContext); + + // Verify + assertNotNull("ValueStack should be returned", result); + assertTrue("Warning about direct JSP access should be logged", + hasWarningLogMessage("Rendering tag out of Action scope")); + } + + public void testGetStack_withNullAction_logsWarning() { + // Setup: ValueStack and ActionInvocation exist but action is null + ValueStack stack = ActionContext.getContext().getValueStack(); + request.setAttribute(ServletActionContext.STRUTS_VALUESTACK_KEY, stack); + + MockActionInvocation actionInvocation = new MockActionInvocation(); + actionInvocation.setAction(null); + + ActionContext.of(stack.getContext()) + .withActionInvocation(actionInvocation) + .bind(); + + // Execute + ValueStack result = TagUtils.getStack(pageContext); + + // Verify + assertNotNull("ValueStack should be returned", result); + assertTrue("Warning about direct JSP access should be logged", + hasWarningLogMessage("Rendering tag out of Action scope")); + } + + public void testGetStack_withValidAction_noWarning() { + // Setup: normal action flow with valid action + ValueStack stack = ActionContext.getContext().getValueStack(); + request.setAttribute(ServletActionContext.STRUTS_VALUESTACK_KEY, stack); + + TestAction action = new TestAction(); + MockActionInvocation actionInvocation = new MockActionInvocation(); + actionInvocation.setAction(action); + + ActionContext.of(stack.getContext()) + .withActionInvocation(actionInvocation) + .bind(); + + // Execute + ValueStack result = TagUtils.getStack(pageContext); + + // Verify + assertNotNull("ValueStack should be returned", result); + assertFalse("Warning should NOT be logged when action is present", + hasWarningLogMessage("Rendering tag out of Action scope")); + } + + public void testGetStack_warningMessageContainsSecurityUrl() { + // Setup: ValueStack exists but no ActionInvocation + ValueStack stack = ActionContext.getContext().getValueStack(); + request.setAttribute(ServletActionContext.STRUTS_VALUESTACK_KEY, stack); + + ActionContext.of(stack.getContext()) + .withActionInvocation(null) + .bind(); + + // Execute + TagUtils.getStack(pageContext); + + // Verify warning contains security documentation URL + assertTrue("Warning should contain security documentation URL", + hasWarningLogMessage("https://struts.apache.org/security/#never-expose-jsp-files-directly";)); + } + + private boolean hasWarningLogMessage(String messageSubstring) { + return testAppender.logEvents.stream() + .anyMatch(event -> event.getLevel() == Level.WARN + && event.getMessage().getFormattedMessage().contains(messageSubstring)); + } + + /** + * Test appender to capture log events for verification. + */ + static class TestAppender extends AbstractAppender { + List<LogEvent> logEvents = new ArrayList<>(); + + TestAppender() { + super("TestAppender", null, null, false, null); + } + + @Override + public void append(LogEvent logEvent) { + logEvents.add(logEvent.toImmutable()); + } + } +}
