Merge pull request #1108 from apache/WW-5480-templating-warning

WW-5480 Warn against potential templating bug

core/src/main/java/org/apache/struts2/components/UIBean.java

@@ -655,7 +655,7 @@ public void evaluateParams() {
         if (this.key != null) {
 
             if(this.name == null) {
-                this.name = key;
+                setName(key);
             }
 
             if(this.label == null) {
@@ -1137,6 +1137,13 @@ public void setErrorPosition(String errorPosition) {
 
     @StrutsTagAttribute(description="The name to set for element")
     public void setName(String name) {
+        if (name != null && name.startsWith("$")) {
+            LOG.error("The name attribute should not usually be a templating variable." +
+                      " This can cause a critical vulnerability if the resolved value is derived from user input." +
+                      " If you are certain that you require this behaviour, please use OGNL expression syntax ( %{expr} ) instead.",
+                    new IllegalStateException());
+            return;
+        }
         this.name = name;
     }
 

core/src/test/java/org/apache/struts2/components/UIBeanTest.java

@@ -38,6 +38,22 @@
 
 public class UIBeanTest extends StrutsInternalTestCase {
 
+    private UIBean bean;
+
+    @Override
+    public void setUp() throws Exception {
+        super.setUp();
+        ValueStack stack = ActionContext.getContext().getValueStack();
+        MockHttpServletRequest req = new MockHttpServletRequest();
+        MockHttpServletResponse res = new MockHttpServletResponse();
+        bean = new UIBean(stack, req, res) {
+            @Override
+            protected String getDefaultTemplate() {
+                return null;
+            }
+        };
+    }
+
     public void testPopulateComponentHtmlId1() {
         ValueStack stack = ActionContext.getContext().getValueStack();
         MockHttpServletRequest req = new MockHttpServletRequest();
@@ -102,15 +118,6 @@ public void testPopulateComponentHtmlWithoutNameAndId() {
     }
 
     public void testEscape() {
-        ValueStack stack = ActionContext.getContext().getValueStack();
-        MockHttpServletRequest req = new MockHttpServletRequest();
-        MockHttpServletResponse res = new MockHttpServletResponse();
-        UIBean bean = new UIBean(stack, req, res) {
-            protected String getDefaultTemplate() {
-                return null;
-            }
-        };
-
         assertEquals(bean.escape("hello[world"), "hello_world");
         assertEquals(bean.escape("hello.world"), "hello_world");
         assertEquals(bean.escape("hello]world"), "hello_world");
@@ -424,4 +431,27 @@ public void testSetNullUiStaticContentPath() {
         assertEquals("/content", field.uiStaticContentPath);
     }
 
+    /**
+     * The {@code name} attribute of a {@link UIBean} is evaluated to determine the {@value UIBean#ATTR_NAME_VALUE}
+     * parameter value. Thus, it is imperative that the {@code name} attribute is not derived from user input as it will
+     * otherwise result in a critical SSTI vulnerability.
+     * <p>
+     * When using FreeMarker, if the {@code name} attribute is a templating variable that corresponds to a getter which
+     * returns user-controlled input, it will usually resolve to {@code null} when loading the corresponding Action,
+     * which results in a rendering error, giving developers strong feedback that the attribute is not set correctly.
+     * <p>
+     * In the case of Velocity, templating variables which resolve to {@code null} do not cause rendering errors, making
+     * this potentially critical mistake sometimes undetectable. By logging a prominent warning, Velocity developers are
+     * also given a clear indication that the {@code name} attribute is not set correctly.
+     * <p>
+     * If the name attribute should definitely correspond to a variable (it is NOT derived from user input), the warning
+     * can be suppressed by using the Struts OGNL expression syntax instead ( %{expr} ). This may be appropriate when
+     * defining Struts components within an Iterator or loop.
+     */
+    public void testPotentialDoubleEvaluationWarning() {
+        bean.setName("${someVar}");
+
+        assertNull(bean.name);
+    }
+
 }