Merge pull request #425 from apache/WW-5077-dmi-aware

[WW-5077] DMI aware pattern
This commit is contained in:
Yasser Zamani
2020-07-17 14:16:36 +04:30
committed by GitHub
3 changed files with 104 additions and 57 deletions
@@ -21,8 +21,10 @@ package com.opensymphony.xwork2.security;
import com.opensymphony.xwork2.XWorkConstants;
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.util.TextParseUtil;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
import java.util.Arrays;
import java.util.Collections;
@@ -35,7 +37,11 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker {
private static final Logger LOG = LogManager.getLogger(DefaultAcceptedPatternsChecker.class);
public static final String[] ACCEPTED_PATTERNS = {
"\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\\u9fa5])+'\\])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*"
"\\w+((\\.\\w+)|(\\[\\d+])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\\u9fa5])+'])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*"
};
public static final String[] DMI_AWARE_ACCEPTED_PATTERNS = {
"\\w+([:]?\\w+)?((\\.\\w+)|(\\[\\d+])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\\u9fa5])+'])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*([!]?\\w+)?"
};
private Set<Pattern> acceptedPatterns;
@@ -44,10 +50,21 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker {
setAcceptedPatterns(ACCEPTED_PATTERNS);
}
public DefaultAcceptedPatternsChecker(
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false) String dmiValue
) {
if (BooleanUtils.toBoolean(dmiValue)) {
LOG.debug("DMI is enabled, adding DMI related accepted patterns");
setAcceptedPatterns(DMI_AWARE_ACCEPTED_PATTERNS);
} else {
setAcceptedPatterns(ACCEPTED_PATTERNS);
}
}
@Inject(value = XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, required = false)
protected void setOverrideAcceptedPatterns(String acceptablePatterns) {
LOG.warn("Overriding accepted patterns [{}] with [{}], be aware that this affects all instances and safety of your application!",
acceptedPatterns, acceptablePatterns);
acceptedPatterns, acceptablePatterns);
acceptedPatterns = new HashSet<>();
try {
for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptablePatterns)) {
@@ -88,7 +105,7 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker {
LOG.debug("Sets accepted patterns to [{}], note this impacts the safety of your application!", patterns);
} else {
LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and safety of your application!",
acceptedPatterns, patterns);
acceptedPatterns, patterns);
}
acceptedPatterns = new HashSet<>(patterns.size());
try {
@@ -81,7 +81,7 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker {
@Inject(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION)
protected void setDynamicMethodInvocation(String dmiValue) {
if (BooleanUtils.toBoolean(dmiValue) == false) {
if (!BooleanUtils.toBoolean(dmiValue)) {
LOG.debug("DMI is disabled, adding DMI related excluded patterns");
setAdditionalExcludePatterns("^(action|method):.*");
}
@@ -27,41 +27,41 @@ import java.util.regex.Pattern;
public class DefaultAcceptedPatternsCheckerTest extends XWorkTestCase {
public void testHardcodedAcceptedPatterns() throws Exception {
// given
List<String> params = new ArrayList<String>() {
{
add("%{#application['test']}");
add("%{#application.test}");
add("%{#Application['test']}");
add("%{#Application.test}");
add("%{#session['test']}");
add("%{#session.test}");
add("%{#Session['test']}");
add("%{#Session.test}");
add("%{#struts['test']}");
add("%{#struts.test}");
add("%{#Struts['test']}");
add("%{#Struts.test}");
add("%{#request['test']}");
add("%{#request.test}");
add("%{#Request['test']}");
add("%{#Request.test}");
add("%{#servletRequest['test']}");
add("%{#servletRequest.test}");
add("%{#ServletRequest['test']}");
add("%{#ServletRequest.test}");
add("%{#servletResponse['test']}");
add("%{#servletResponse.test}");
add("%{#ServletResponse['test']}");
add("%{#ServletResponse.test}");
add("%{#parameters['test']}");
add("%{#parameters.test}");
add("%{#Parameters['test']}");
add("%{#Parameters.test}");
}
};
private final List<String> params = new ArrayList<String>() {
{
add("%{#application['test']}");
add("%{#application.test}");
add("%{#Application['test']}");
add("%{#Application.test}");
add("%{#session['test']}");
add("%{#session.test}");
add("%{#Session['test']}");
add("%{#Session.test}");
add("%{#struts['test']}");
add("%{#struts.test}");
add("%{#Struts['test']}");
add("%{#Struts.test}");
add("%{#request['test']}");
add("%{#request.test}");
add("%{#Request['test']}");
add("%{#Request.test}");
add("%{#servletRequest['test']}");
add("%{#servletRequest.test}");
add("%{#ServletRequest['test']}");
add("%{#ServletRequest.test}");
add("%{#servletResponse['test']}");
add("%{#servletResponse.test}");
add("%{#ServletResponse['test']}");
add("%{#ServletResponse.test}");
add("%{#parameters['test']}");
add("%{#parameters.test}");
add("%{#Parameters['test']}");
add("%{#Parameters.test}");
}
};
public void testHardcodedAcceptedPatterns() {
// given
AcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker();
for (String param : params) {
@@ -73,7 +73,20 @@ public class DefaultAcceptedPatternsCheckerTest extends XWorkTestCase {
}
}
public void testUnderscoreInParamName() throws Exception {
public void testHardcodedAcceptedPatternsWithDmiEnabled() {
// given
AcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker(Boolean.TRUE.toString());
for (String param : params) {
// when
AcceptedPatternsChecker.IsAccepted actual = checker.isAccepted(param);
// then
assertFalse("Access to " + param + " is possible!", actual.isAccepted());
}
}
public void testUnderscoreInParamName() {
// given
AcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker();
@@ -84,22 +97,33 @@ public class DefaultAcceptedPatternsCheckerTest extends XWorkTestCase {
assertTrue("Param with underscore wasn't accepted!", actual.isAccepted());
}
public void testAcceptedPatternsImmutable() throws Exception {
public void testUnderscoreInParamNameWithDmiEnabled() {
// given
AcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker(Boolean.TRUE.toString());
// when
AcceptedPatternsChecker.IsAccepted actual = checker.isAccepted("mapParam['param_1']");
// then
assertTrue("Param with underscore wasn't accepted!", actual.isAccepted());
}
public void testAcceptedPatternsImmutable() {
AcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker();
Set<Pattern> acceptedPatternSet = checker.getAcceptedPatterns();
assertNotNull("default accepted patterns null?", acceptedPatternSet);
assertFalse("default accepted patterns empty?", acceptedPatternSet.isEmpty());
try {
acceptedPatternSet.add(Pattern.compile("SomeRegexPattern") );
fail ("accepted patterns modifiable?");
} catch(UnsupportedOperationException uoe) {
acceptedPatternSet.add(Pattern.compile("SomeRegexPattern"));
fail("accepted patterns modifiable?");
} catch (UnsupportedOperationException uoe) {
// Expected result
}
try {
acceptedPatternSet.clear();
fail ("accepted patterns modifiable?");
} catch(UnsupportedOperationException uoe) {
fail("accepted patterns modifiable?");
} catch (UnsupportedOperationException uoe) {
// Expected result
}
@@ -108,15 +132,15 @@ public class DefaultAcceptedPatternsCheckerTest extends XWorkTestCase {
assertNotNull("replaced default accepted patterns null?", acceptedPatternSet);
assertFalse("replaced default accepted patterns empty?", acceptedPatternSet.isEmpty());
try {
acceptedPatternSet.add(Pattern.compile("SomeRegexPattern") );
fail ("replaced accepted patterns modifiable?");
} catch(UnsupportedOperationException uoe) {
acceptedPatternSet.add(Pattern.compile("SomeRegexPattern"));
fail("replaced accepted patterns modifiable?");
} catch (UnsupportedOperationException uoe) {
// Expected result
}
try {
acceptedPatternSet.clear();
fail ("accepted patterns modifiable?");
} catch(UnsupportedOperationException uoe) {
fail("accepted patterns modifiable?");
} catch (UnsupportedOperationException uoe) {
// Expected result
}
@@ -125,23 +149,29 @@ public class DefaultAcceptedPatternsCheckerTest extends XWorkTestCase {
acceptedPatternSet = checker.getAcceptedPatterns();
assertNotNull("replaced default accepted patterns null?", acceptedPatternSet);
assertFalse("replaced default accepted patterns empty?", acceptedPatternSet.isEmpty());
assertTrue("replaced default accepted patterns not size " + testPatternArray.length + "?",
acceptedPatternSet.size() == testPatternArray.length);
assertEquals("replaced default accepted patterns not size " + testPatternArray.length + "?", acceptedPatternSet.size(), testPatternArray.length);
for (String testPatternArray1 : testPatternArray) {
assertTrue(testPatternArray1 + " not accepted?", checker.isAccepted(testPatternArray1).isAccepted());
}
try {
acceptedPatternSet.add(Pattern.compile("SomeRegexPattern") );
fail ("replaced accepted patterns modifiable?");
} catch(UnsupportedOperationException uoe) {
acceptedPatternSet.add(Pattern.compile("SomeRegexPattern"));
fail("replaced accepted patterns modifiable?");
} catch (UnsupportedOperationException uoe) {
// Expected result
}
try {
acceptedPatternSet.clear();
fail ("accepted patterns modifiable?");
} catch(UnsupportedOperationException uoe) {
fail("accepted patterns modifiable?");
} catch (UnsupportedOperationException uoe) {
// Expected result
}
}
public void testDmiIsEnabled() {
DefaultAcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker(Boolean.TRUE.toString());
AcceptedPatternsChecker.IsAccepted accepted = checker.isAccepted("action:myAction!save");
assertTrue("dmi isn't accepted", accepted.isAccepted());
}
}