Merge pull request #480 from apache/WW-5117-reorders-stack

[WW-5117] Keeps Action always on top of the ValueStack

core/src/main/java/org/apache/struts2/components/template/FreemarkerTemplateEngine.java

@@ -64,7 +64,7 @@ public class FreemarkerTemplateEngine extends BaseTemplateEngine {
     public void setFreemarkerManager(FreemarkerManager mgr) {
         this.freemarkerManager = mgr;
     }
-    
+
     public void renderTemplate(TemplateRenderingContext templateContext) throws Exception {
     	// get the various items required from the stack
         ValueStack stack = templateContext.getStack();
@@ -121,6 +121,10 @@ public class FreemarkerTemplateEngine extends BaseTemplateEngine {
         ActionInvocation ai = ActionContext.getContext().getActionInvocation();
 
         Object action = (ai == null) ? null : ai.getAction();
+        if (action == 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", templateName);
+        }
         SimpleHash model = freemarkerManager.buildTemplateModel(stack, action, servletContext, req, res, config.getObjectWrapper());
 
         model.put("tag", templateContext.getTag());
@@ -144,15 +148,20 @@ public class FreemarkerTemplateEngine extends BaseTemplateEngine {
             }
         };
 
+        LOG.debug("Puts action on the top of ValueStack, just before the tag");
+        action = stack.pop();
+        stack.push(templateContext.getTag());
+        stack.push(action);
         try {
-            stack.push(templateContext.getTag());
             template.process(model, writer);
         } finally {
-            stack.pop();
+            stack.pop(); // removes action
+            stack.pop(); // removes tag
+            stack.push(action); // puts back action
         }
     }
 
     protected String getSuffix() {
         return "ftl";
     }
-}
+}

core/src/main/resources/template/simple/dynamic-attributes.ftl

@@ -29,4 +29,4 @@
   </#if>
  ${aKey}="${value}"<#rt/>
 </#list><#rt/>
-</#if><#rt/>
+</#if><#rt/>

core/src/test/java/org/apache/struts2/TestAction.java

@@ -49,6 +49,7 @@ public class TestAction extends ActionSupport {
     private List list3;
     private SomeEnum status = SomeEnum.COMPLETED;
     private Float floatNumber;
+    private Long id;
 
     private final Map<String, String> texts = new HashMap<String, String>();
 
@@ -213,7 +214,7 @@ public class TestAction extends ActionSupport {
 	public void setStatus(SomeEnum status) {
 		this.status = status;
 	}
-    
+
     public List<SomeEnum> getStatusList() {
     	return Arrays.asList(SomeEnum.values());
     }
@@ -225,4 +226,13 @@ public class TestAction extends ActionSupport {
     public void setFloatNumber(Float floatNumber) {
         this.floatNumber = floatNumber;
     }
+
+    public Long getId() {
+        return id;
+    }
+
+    public void setId(Long id) {
+        this.id = id;
+    }
+
 }

core/src/test/java/org/apache/struts2/views/jsp/ui/HiddenTest.java

@@ -18,15 +18,14 @@
  */
 package org.apache.struts2.views.jsp.ui;
 
-import java.util.HashMap;
-import java.util.Map;
-
+import com.opensymphony.xwork2.ActionContext;
+import com.opensymphony.xwork2.mock.MockActionInvocation;
 import org.apache.struts2.TestAction;
 import org.apache.struts2.views.jsp.AbstractUITagTest;
 
+import java.util.HashMap;
+import java.util.Map;
 
-/**
- */
 public class HiddenTest extends AbstractUITagTest {
 
     public void testSimple() throws Exception {
@@ -62,13 +61,57 @@ public class HiddenTest extends AbstractUITagTest {
         verify(TextFieldTag.class.getResource("Hidden-2.txt"));
     }
 
+    public void testDynamicAttributesWithActionInvocation() throws Exception {
+        TestAction testAction = (TestAction) action;
+        testAction.setId(27357L);
+
+        MockActionInvocation ai = new MockActionInvocation();
+        ai.setAction(action);
+        ActionContext.getContext().setActionInvocation(ai);
+
+        HiddenTag tag = new HiddenTag();
+        tag.setPageContext(pageContext);
+        tag.setId("einszwei");
+        tag.setName("first");
+        tag.setValue("%{id}");
+        tag.setDynamicAttribute("", "data-wuffmiauww", "%{id}");
+
+        tag.doStartTag();
+        tag.doEndTag();
+
+        assertSame(stack.pop(), testAction);
+        assertNotSame(stack.pop(), tag);
+
+        verify(TextFieldTag.class.getResource("Hidden-3.txt"));
+    }
+
+    public void testDynamicAttributesWithStack() throws Exception {
+        TestAction testAction = (TestAction) action;
+        testAction.setId(27357L);
+
+        HiddenTag tag = new HiddenTag();
+        tag.setPageContext(pageContext);
+        tag.setId("einszwei");
+        tag.setName("first");
+        tag.setValue("%{id}");
+        tag.setDynamicAttribute("", "data-wuffmiauww", "%{id}");
+
+        tag.doStartTag();
+        tag.doEndTag();
+
+        assertSame(stack.pop(), testAction);
+        assertNotSame(stack.pop(), tag);
+
+        verify(TextFieldTag.class.getResource("Hidden-3.txt"));
+    }
+
     /**
      * Initialize a map of {@link org.apache.struts2.views.jsp.AbstractUITagTest.PropertyHolder} for generic tag
      * property testing. Will be used when calling {@link #verifyGenericProperties(org.apache.struts2.views.jsp.ui.AbstractUITag,
      * String, String[])} as properties to verify.<br> This implementation extends testdata from AbstractUITag.
      *
      * @return A Map of PropertyHolders values bound to {@link org.apache.struts2.views.jsp.AbstractUITagTest.PropertyHolder#getName()}
-     *         as key.
+     * as key.
      */
     protected Map initializedGenericTagTestProperties() {
         Map result = new HashMap();

core/src/test/resources/org/apache/struts2/views/jsp/ui/Hidden-3.txt

@@ -0,0 +1,5 @@
+<tr style="display:none;">
+  <td colspan="2">
+    <input type="hidden" name="first" value="27357" id="einszwei" data-wuffmiauww="27357"/>
+  </td>
+</tr>