STR-3169: Do not match when recursive substitutions are detected
git-svn-id: https://svn.apache.org/repos/asf/struts/struts1/trunk@728474 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/core/src/main/java/org/apache/struts/config/ActionConfigMatcher.java b/core/src/main/java/org/apache/struts/config/ActionConfigMatcher.java
index 3d22251..5c75b94 100755
--- a/core/src/main/java/org/apache/struts/config/ActionConfigMatcher.java
+++ b/core/src/main/java/org/apache/struts/config/ActionConfigMatcher.java
@@ -123,9 +123,18 @@
+ m.getActionConfig().getPath() + "'");
}
- config =
- convertActionConfig(path,
- (ActionConfig) m.getActionConfig(), vars);
+ try {
+ config =
+ convertActionConfig(path,
+ (ActionConfig) m.getActionConfig(), vars);
+ } catch (IllegalStateException e) {
+ log.warn("Path matches pattern '"
+ + m.getActionConfig().getPath() + "' but is "
+ + "incompatible with the matching config due "
+ + "to recursive substitution: "
+ + path);
+ config = null;
+ }
}
}
}
@@ -221,6 +230,8 @@
* @param orig The original properties set with placehold values
* @param props The target properties to store the processed values
* @param vars A Map of wildcard-matched strings
+ * @throws IllegalStateException if a placeholder substitution is
+ * impossible due to recursion
*/
protected void replaceProperties(Properties orig, Properties props, Map vars) {
Map.Entry entry = null;
@@ -239,6 +250,8 @@
* @param val The value to convert
* @param vars A Map of wildcard-matched strings
* @return The new value
+ * @throws IllegalStateException if a placeholder substitution is
+ * impossible due to recursion
*/
protected String convertParam(String val, Map vars) {
if (val == null) {
@@ -250,16 +263,23 @@
Map.Entry entry;
StringBuffer key = new StringBuffer("{0}");
StringBuffer ret = new StringBuffer(val);
- String keyTmp;
+ String keyStr;
int x;
for (Iterator i = vars.entrySet().iterator(); i.hasNext();) {
entry = (Map.Entry) i.next();
key.setCharAt(1, ((String) entry.getKey()).charAt(0));
- keyTmp = key.toString();
-
+ keyStr = key.toString();
+
+ // STR-3169
+ // Prevent an infinite loop by retaining the placeholders
+ // that contain itself in the substitution value
+ if (((String) entry.getValue()).contains(keyStr)) {
+ throw new IllegalStateException();
+ }
+
// Replace all instances of the placeholder
- while ((x = ret.toString().indexOf(keyTmp)) > -1) {
+ while ((x = ret.toString().indexOf(keyStr)) > -1) {
ret.replace(x, x + 3, (String) entry.getValue());
}
}
@@ -311,4 +331,5 @@
return this.config;
}
}
+
}
diff --git a/core/src/test/java/org/apache/struts/config/TestActionConfigMatcher.java b/core/src/test/java/org/apache/struts/config/TestActionConfigMatcher.java
index 9cc6d70..4dc9181 100755
--- a/core/src/test/java/org/apache/struts/config/TestActionConfigMatcher.java
+++ b/core/src/test/java/org/apache/struts/config/TestActionConfigMatcher.java
@@ -27,204 +27,270 @@
import org.apache.struts.action.ActionMapping;
import org.apache.struts.mock.TestMockBase;
+import java.util.HashMap;
+
/**
- * <p>Unit tests for <code>org.apache.struts.util.ActionConfigMatcher</code>.</p>
- *
- * @version $Rev$ $Date: 2005-10-27 23:25:01 -0400 (Thu, 27 Oct 2005)
- * $
+ * <p>
+ * Unit tests for <code>org.apache.struts.util.ActionConfigMatcher</code>.
+ * </p>
+ *
+ * @version $Rev$ $Date$
*/
public class TestActionConfigMatcher extends TestMockBase {
// ----------------------------------------------------------------- Basics
public TestActionConfigMatcher(String name) {
- super(name);
+ super(name);
}
public static void main(String[] args) {
- junit.awtui.TestRunner.main(new String[] {
- TestActionConfigMatcher.class.getName()
- });
+ junit.awtui.TestRunner.main(new String[] { TestActionConfigMatcher.class.getName() });
}
public static Test suite() {
- return (new TestSuite(TestActionConfigMatcher.class));
+ return (new TestSuite(TestActionConfigMatcher.class));
}
// ----------------------------------------------------- Instance Variables
// ----------------------------------------------------- Setup and Teardown
public void setUp() {
- super.setUp();
+ super.setUp();
}
public void tearDown() {
- super.tearDown();
+ super.tearDown();
}
// ------------------------------------------------------- Individual Tests
// ---------------------------------------------------------- match()
public void testNoMatch() {
- ActionConfig[] configs = new ActionConfig[1];
- ActionConfig mapping = buildActionConfig("/foo");
+ ActionConfig[] configs = new ActionConfig[1];
+ ActionConfig mapping = buildActionConfig("/foo");
- configs[0] = mapping;
+ configs[0] = mapping;
- ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
+ ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
- assertNull("ActionConfig shouldn't be matched", matcher.match("/test"));
+ assertNull("ActionConfig shouldn't be matched", matcher.match("/test"));
+ }
+
+ /**
+ * Verifies that a match succeeds when the substituted value contains a
+ * placeholder key.
+ * <p>
+ * See STR-3169.
+ *
+ * @since Struts 1.3.11
+ */
+ public void testMatchWithKeyInPath() {
+ ActionMapping[] mapping = new ActionMapping[1];
+ mapping[0] = new ActionMapping();
+ mapping[0].setPath("/page-*");
+
+ ActionConfigMatcher matcher = new ActionConfigMatcher(mapping);
+ ActionConfig matched = matcher.match("/page-{0}");
+
+ assertNotNull("ActionConfig should be matched", matched);
+ assertEquals("Path hasn't been replaced", "/page-{0}", matched.getPath());
+ }
+
+ /**
+ * Verifies that an infinite loop is prevented and substitution is skipped when
+ * the substituted value equals the placeholder key.
+ * <p>
+ * See STR-3169.
+ *
+ * @since Struts 1.3.11
+ * @see #testMatchWithSubstitutionEqualPlaceholderKey1()
+ */
+ public void testMatchWithSubstitutionEqualPlaceholderKey0() {
+ ActionMapping[] mapping = new ActionMapping[1];
+ mapping[0] = new ActionMapping();
+ mapping[0].setPath("/page-*");
+ mapping[0].setParameter("{0}");
+
+ ActionConfigMatcher matcher = new ActionConfigMatcher(mapping);
+ ActionConfig matched = matcher.match("/page-{1}");
+
+ assertNull("ActionConfig should not be matched", matched);
+ }
+
+ /**
+ * Verifies that an infinite loop is prevented and substitution is skipped when
+ * the substituted value equals the placeholder key.
+ * <p>
+ * See STR-3169.
+ *
+ * @since Struts 1.3.11
+ * @see #testMatchWithSubstitutionEqualPlaceholderKey0()
+ */
+ public void testMatchWithSubstitutionEqualPlaceholderKey1() {
+ ActionMapping[] mapping = new ActionMapping[1];
+ mapping[0] = new ActionMapping();
+ mapping[0].setPath("/page-*");
+ mapping[0].setParameter("{1}");
+
+ ActionConfigMatcher matcher = new ActionConfigMatcher(mapping);
+ ActionConfig matched = matcher.match("/page-{1}");
+
+ assertNull("ActionConfig should not be matched", matched);
+ }
+
+ /**
+ * Verifies that an infinite loop is prevented and substitution is skipped when
+ * the the placeholder key is contained within the substituted value.
+ * <p>
+ * See STR-3169.
+ *
+ * @since Struts 1.3.11
+ */
+ public void testMatchWhenSubstitutionContainsPlaceholderKey() {
+ ActionMapping[] mapping = new ActionMapping[1];
+ mapping[0] = new ActionMapping();
+ mapping[0].setPath("/page-*");
+ mapping[0].setParameter("{1}");
+
+ ActionConfigMatcher matcher = new ActionConfigMatcher(mapping);
+ ActionConfig matched = matcher.match("/page-{0}");
+
+ assertNull("ActionConfig should not be matched", matched);
}
public void testNoWildcardMatch() {
- ActionConfig[] configs = new ActionConfig[1];
- ActionConfig mapping = buildActionConfig("/fooBar");
+ ActionConfig[] configs = new ActionConfig[1];
+ ActionConfig mapping = buildActionConfig("/fooBar");
- configs[0] = mapping;
+ configs[0] = mapping;
- ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
+ ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
- assertNull("ActionConfig shouldn't be matched", matcher.match("/fooBar"));
+ assertNull("ActionConfig shouldn't be matched", matcher.match("/fooBar"));
}
public void testShouldMatch() {
- ActionConfig[] configs = new ActionConfig[1];
- ActionConfig mapping = buildActionConfig("/foo*");
+ ActionConfig[] configs = new ActionConfig[1];
+ ActionConfig mapping = buildActionConfig("/foo*");
- configs[0] = mapping;
+ configs[0] = mapping;
- ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
+ ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
- ActionConfig matched = matcher.match("/fooBar");
+ ActionConfig matched = matcher.match("/fooBar");
- assertNotNull("ActionConfig should be matched", matched);
- assertTrue("ActionConfig should have two action forward",
- matched.findForwardConfigs().length == 2);
- assertTrue("ActionConfig should have two exception forward",
- matched.findExceptionConfigs().length == 2);
- assertTrue("ActionConfig should have properties",
- matched.getProperties().size() == 2);
+ assertNotNull("ActionConfig should be matched", matched);
+ assertTrue("ActionConfig should have two action forward", matched.findForwardConfigs().length == 2);
+ assertTrue("ActionConfig should have two exception forward", matched.findExceptionConfigs().length == 2);
+ assertTrue("ActionConfig should have properties", matched.getProperties().size() == 2);
}
public void testCheckSubstitutionsMatch() {
- ActionConfig[] configs = new ActionConfig[1];
- ActionConfig mapping = buildActionConfig("/foo*");
+ ActionConfig[] configs = new ActionConfig[1];
+ ActionConfig mapping = buildActionConfig("/foo*");
- configs[0] = mapping;
+ configs[0] = mapping;
- ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
- ActionConfig m = matcher.match("/fooBar");
+ ActionConfigMatcher matcher = new ActionConfigMatcher(configs);
+ ActionConfig m = matcher.match("/fooBar");
- assertTrue("Name hasn't been replaced", "name,Bar".equals(m.getName()));
- assertTrue("Path hasn't been replaced", "/fooBar".equals(m.getPath()));
- assertTrue("Scope isn't correct", "request".equals(m.getScope()));
- assertTrue("Unknown isn't correct", !m.getUnknown());
- assertTrue("Validate isn't correct", m.getValidate());
+ assertTrue("Name hasn't been replaced", "name,Bar".equals(m.getName()));
+ assertTrue("Path hasn't been replaced", "/fooBar".equals(m.getPath()));
+ assertTrue("Scope isn't correct", "request".equals(m.getScope()));
+ assertTrue("Unknown isn't correct", !m.getUnknown());
+ assertTrue("Validate isn't correct", m.getValidate());
- assertTrue("Prefix hasn't been replaced",
- "foo,Bar".equals(m.getPrefix()));
- assertTrue("Suffix hasn't been replaced",
- "bar,Bar".equals(m.getSuffix()));
- assertTrue("Type hasn't been replaced",
- "foo.bar.BarAction".equals(m.getType()));
- assertTrue("Roles hasn't been replaced",
- "public,Bar".equals(m.getRoles()));
- assertTrue("Parameter hasn't been replaced",
- "param,Bar".equals(m.getParameter()));
- assertTrue("Attribute hasn't been replaced",
- "attrib,Bar".equals(m.getAttribute()));
- assertTrue("Forward hasn't been replaced",
- "fwd,Bar".equals(m.getForward()));
- assertTrue("Include hasn't been replaced",
- "include,Bar".equals(m.getInclude()));
- assertTrue("Input hasn't been replaced",
- "input,Bar".equals(m.getInput()));
+ assertTrue("Prefix hasn't been replaced", "foo,Bar".equals(m.getPrefix()));
+ assertTrue("Suffix hasn't been replaced", "bar,Bar".equals(m.getSuffix()));
+ assertTrue("Type hasn't been replaced", "foo.bar.BarAction".equals(m.getType()));
+ assertTrue("Roles hasn't been replaced", "public,Bar".equals(m.getRoles()));
+ assertTrue("Parameter hasn't been replaced", "param,Bar".equals(m.getParameter()));
+ assertTrue("Attribute hasn't been replaced", "attrib,Bar".equals(m.getAttribute()));
+ assertTrue("Forward hasn't been replaced", "fwd,Bar".equals(m.getForward()));
+ assertTrue("Include hasn't been replaced", "include,Bar".equals(m.getInclude()));
+ assertTrue("Input hasn't been replaced", "input,Bar".equals(m.getInput()));
- assertTrue("ActionConfig property not replaced",
- "testBar".equals(m.getProperty("testprop2")));
+ assertTrue("ActionConfig property not replaced", "testBar".equals(m.getProperty("testprop2")));
- ForwardConfig[] fConfigs = m.findForwardConfigs();
- boolean found = false;
+ ForwardConfig[] fConfigs = m.findForwardConfigs();
+ boolean found = false;
- for (int x = 0; x < fConfigs.length; x++) {
- ForwardConfig cfg = fConfigs[x];
+ for (int x = 0; x < fConfigs.length; x++) {
+ ForwardConfig cfg = fConfigs[x];
- if ("name".equals(cfg.getName())) {
- found = true;
- assertTrue("Path hasn't been replaced",
- "path,Bar".equals(cfg.getPath()));
- assertTrue("Property foo hasn't been replaced",
- "bar,Bar".equals(cfg.getProperty("foo")));
- assertTrue("Module hasn't been replaced",
- "modBar".equals(cfg.getModule()));
- }
- }
+ if ("name".equals(cfg.getName())) {
+ found = true;
+ assertTrue("Path hasn't been replaced", "path,Bar".equals(cfg.getPath()));
+ assertTrue("Property foo hasn't been replaced", "bar,Bar".equals(cfg.getProperty("foo")));
+ assertTrue("Module hasn't been replaced", "modBar".equals(cfg.getModule()));
+ }
+ }
- assertTrue("The forward config 'name' cannot be found", found);
+ assertTrue("The forward config 'name' cannot be found", found);
}
public void testCheckMultipleSubstitutions() {
- ActionMapping[] mapping = new ActionMapping[1];
+ ActionMapping[] mapping = new ActionMapping[1];
- mapping[0] = new ActionMapping();
- mapping[0].setPath("/foo*");
- mapping[0].setName("name,{1}-{1}");
+ mapping[0] = new ActionMapping();
+ mapping[0].setPath("/foo*");
+ mapping[0].setName("name,{1}-{1}");
- ActionConfigMatcher matcher = new ActionConfigMatcher(mapping);
- ActionConfig m = matcher.match("/fooBar");
+ ActionConfigMatcher matcher = new ActionConfigMatcher(mapping);
+ ActionConfig m = matcher.match("/fooBar");
- assertTrue("Name hasn't been replaced correctly: " + m.getName(),
- "name,Bar-Bar".equals(m.getName()));
+ assertTrue("Name hasn't been replaced correctly: " + m.getName(), "name,Bar-Bar".equals(m.getName()));
}
private ActionConfig buildActionConfig(String path) {
- ActionMapping mapping = new ActionMapping();
+ ActionMapping mapping = new ActionMapping();
- mapping.setName("name,{1}");
- mapping.setPath(path);
- mapping.setScope("request");
- mapping.setUnknown(false);
- mapping.setValidate(true);
+ mapping.setName("name,{1}");
+ mapping.setPath(path);
+ mapping.setScope("request");
+ mapping.setUnknown(false);
+ mapping.setValidate(true);
- mapping.setPrefix("foo,{1}");
- mapping.setSuffix("bar,{1}");
+ mapping.setPrefix("foo,{1}");
+ mapping.setSuffix("bar,{1}");
- mapping.setType("foo.bar.{1}Action");
- mapping.setRoles("public,{1}");
- mapping.setParameter("param,{1}");
- mapping.setAttribute("attrib,{1}");
- mapping.setForward("fwd,{1}");
- mapping.setInclude("include,{1}");
- mapping.setInput("input,{1}");
+ mapping.setType("foo.bar.{1}Action");
+ mapping.setRoles("public,{1}");
+ mapping.setParameter("param,{1}");
+ mapping.setAttribute("attrib,{1}");
+ mapping.setForward("fwd,{1}");
+ mapping.setInclude("include,{1}");
+ mapping.setInput("input,{1}");
- ForwardConfig cfg = new ActionForward();
+ ForwardConfig cfg = new ActionForward();
- cfg.setName("name");
- cfg.setPath("path,{1}");
- cfg.setModule("mod{1}");
- cfg.setProperty("foo", "bar,{1}");
- mapping.addForwardConfig(cfg);
+ cfg.setName("name");
+ cfg.setPath("path,{1}");
+ cfg.setModule("mod{1}");
+ cfg.setProperty("foo", "bar,{1}");
+ mapping.addForwardConfig(cfg);
- cfg = new ActionForward();
- cfg.setName("name2");
- cfg.setPath("path2");
- cfg.setModule("mod{1}");
- mapping.addForwardConfig(cfg);
+ cfg = new ActionForward();
+ cfg.setName("name2");
+ cfg.setPath("path2");
+ cfg.setModule("mod{1}");
+ mapping.addForwardConfig(cfg);
- ExceptionConfig excfg = new ExceptionConfig();
+ ExceptionConfig excfg = new ExceptionConfig();
- excfg.setKey("foo");
- excfg.setType("foo.Bar");
- excfg.setPath("path");
- mapping.addExceptionConfig(excfg);
+ excfg.setKey("foo");
+ excfg.setType("foo.Bar");
+ excfg.setPath("path");
+ mapping.addExceptionConfig(excfg);
- excfg = new ExceptionConfig();
- excfg.setKey("foo2");
- excfg.setType("foo.Bar2");
- excfg.setPath("path2");
- mapping.addExceptionConfig(excfg);
+ excfg = new ExceptionConfig();
+ excfg.setKey("foo2");
+ excfg.setType("foo.Bar2");
+ excfg.setPath("path2");
+ mapping.addExceptionConfig(excfg);
- mapping.setProperty("testprop", "testval");
- mapping.setProperty("testprop2", "test{1}");
+ mapping.setProperty("testprop", "testval");
+ mapping.setProperty("testprop2", "test{1}");
- mapping.freeze();
+ mapping.freeze();
- return mapping;
+ return mapping;
}
}
