You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by "kusalk (via GitHub)" <gi...@apache.org> on 2023/08/21 13:40:11 UTC
[GitHub] [struts] kusalk opened a new pull request, #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
kusalk opened a new pull request, #736:
URL: https://github.com/apache/struts/pull/736
WW-5337
--
See comments below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] lukaszlenart commented on a diff in pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #736:
URL: https://github.com/apache/struts/pull/736#discussion_r1300542828
##########
core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java:
##########
@@ -178,23 +178,6 @@ public void testMiddleOfInheritanceExclusion3() throws Exception {
assertTrue("barLogic() from BarInterface isn't accessible!!!", accessible);
}
- @Test
- public void testMiddleOfInheritanceExclusion4() throws Exception {
Review Comment:
I cannot recall the exact use case, yet we can try to relax this behaviour hopping it was just my paranoid mind ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #736:
URL: https://github.com/apache/struts/pull/736#discussion_r1300194447
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -261,7 +262,8 @@ public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses)
}
private Set<String> parseExcludedPackageNames(String commaDelimitedPackageNames) {
- Set<String> parsedSet = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames);
+ Set<String> parsedSet = commaDelimitedStringToSet(commaDelimitedPackageNames)
+ .stream().map(s -> strip(s, ".")).collect(toSet());
Review Comment:
I think it's unintuitive to require a period at the of each excluded package. Currently if the period is missed, the exclusion will act like the regex `com\.package.*` which likely isn't what users configuring their application intend.
By stripping the period here, we retain backwards compatibility for excluded package lists that still have trailing periods.
However, we do drop support for wildcard package exclusions where applications genuinely desired the previous behaviour when a trailing period wasn't included. They will need to move such exclusions to the regex exclusion list instead.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -218,14 +220,13 @@ protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackag
}
private Set<Pattern> parseExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) {
- Set<String> packagePatterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackagePatterns);
- Set<Pattern> packageNamePatterns = new HashSet<>();
-
- for (String pattern : packagePatterns) {
- packageNamePatterns.add(Pattern.compile(pattern));
+ try {
+ return commaDelimitedStringToSet(commaDelimitedPackagePatterns)
+ .stream().map(Pattern::compile).collect(toSet());
+ } catch (PatternSyntaxException e) {
+ throw new ConfigurationException(
Review Comment:
Ensure we throw ConfigurationException when RegEx patterns provided are invalid
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
+ String parentPackage = String.join(".", packageParts.subList(0, i + 1));
+ if (excludedPackageNames.contains(parentPackage)) {
return true;
}
}
return false;
}
protected boolean isClassExcluded(Class<?> clazz) {
- if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) {
- return true;
- }
- return excludedClasses.stream().anyMatch(clazz::isAssignableFrom);
+ return excludedClasses.contains(clazz);
}
protected boolean isExcludedPackageExempt(Class<?> clazz) {
- return excludedPackageExemptClasses.stream().anyMatch(clazz::equals);
+ return excludedPackageExemptClasses.contains(clazz);
Review Comment:
Constant time operation
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
Review Comment:
Operations proportional to number of package parts of class being checked. Previously it was proportional to number of excluded packages in configuration.
Note that we still retain the behaviour of sub-packages also being excluded.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
+ String parentPackage = String.join(".", packageParts.subList(0, i + 1));
+ if (excludedPackageNames.contains(parentPackage)) {
return true;
}
}
return false;
}
protected boolean isClassExcluded(Class<?> clazz) {
- if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) {
Review Comment:
Moved this logic to #useExcludedClasses to make this method more performant
##########
core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java:
##########
@@ -178,23 +178,6 @@ public void testMiddleOfInheritanceExclusion3() throws Exception {
assertTrue("barLogic() from BarInterface isn't accessible!!!", accessible);
}
- @Test
- public void testMiddleOfInheritanceExclusion4() throws Exception {
Review Comment:
Superinterfaces and superclasses of excluded classes are no longer also excluded. I'm not too sure of a real world example where this behaviour might actually be useful/sensible. Or where you wouldn't just add to the exclusion list the desired superinterfaces/superclasses instead?
If this behaviour is genuinely useful, it would be better if we moved such interfaces/classes to a separate exclusion list to keep the regular excluded class list performant.
##########
core/src/main/resources/struts-excluded-classes.xml:
##########
@@ -55,52 +55,52 @@
<!-- constant name="struts.excludedPackageNamePatterns" value="^java\.lang\..*,^ognl.*,^(?!javax\.servlet\..+)(javax\..+)" / -->
<!-- constant name="struts.devMode.excludedPackageNamePatterns" value="^java\.lang\..*,^ognl.*,^(?!javax\.servlet\..+)(javax\..+)" / -->
- <!-- this is simpler version of the above used with string comparison -->
+ <!-- All classes within the following packages and their sub-packages are excluded -->
<constant name="struts.excludedPackageNames"
value="
- ognl.,
- java.io.,
- java.net.,
- java.nio.,
- javax.,
- freemarker.core.,
- freemarker.template.,
- freemarker.ext.jsp.,
- freemarker.ext.rhino.,
- sun.misc.,
- sun.reflect.,
- javassist.,
- org.apache.velocity.,
- org.objectweb.asm.,
- org.springframework.context.,
- com.opensymphony.xwork2.inject.,
- com.opensymphony.xwork2.ognl.,
- com.opensymphony.xwork2.security.,
- com.opensymphony.xwork2.util.,
- org.apache.tomcat.,
- org.apache.catalina.core.,
- org.wildfly.extension.undertow.deployment."/>
+ ognl,
Review Comment:
Whilst trailing periods are still compatible, I've updated this list to abide by the new convention
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -328,11 +327,16 @@ public void useAcceptProperties(Set<Pattern> acceptedProperties) {
*/
@Deprecated
public void setExcludedClasses(Set<Class<?>> excludedClasses) {
- this.excludedClasses = excludedClasses;
+ useExcludedClasses(excludedClasses);
}
public void useExcludedClasses(Set<Class<?>> excludedClasses) {
- this.excludedClasses = excludedClasses;
+ Set<Class<?>> newExcludedClasses = new HashSet<>(excludedClasses);
+ newExcludedClasses.add(Object.class);
Review Comment:
Moved this logic here from #isClassExcluded for performance reasons
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
+ String parentPackage = String.join(".", packageParts.subList(0, i + 1));
+ if (excludedPackageNames.contains(parentPackage)) {
return true;
}
}
return false;
}
protected boolean isClassExcluded(Class<?> clazz) {
- if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) {
- return true;
- }
- return excludedClasses.stream().anyMatch(clazz::isAssignableFrom);
+ return excludedClasses.contains(clazz);
Review Comment:
Constant time operation BUT it changes the semantics, more on this in my comment on `SecurityMemberAccessTest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk merged pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk merged PR #736:
URL: https://github.com/apache/struts/pull/736
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] sonarcloud[bot] commented on pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #736:
URL: https://github.com/apache/struts/pull/736#issuecomment-1686404293
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=736)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=736&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=736&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=736&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_coverage&view=list) [88.5% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #736:
URL: https://github.com/apache/struts/pull/736#discussion_r1300194447
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -261,7 +262,8 @@ public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses)
}
private Set<String> parseExcludedPackageNames(String commaDelimitedPackageNames) {
- Set<String> parsedSet = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames);
+ Set<String> parsedSet = commaDelimitedStringToSet(commaDelimitedPackageNames)
+ .stream().map(s -> strip(s, ".")).collect(toSet());
Review Comment:
I think it's unintuitive to require a period at the of each excluded package. Currently if the period is missed, the exclusion will act like the regex `com\.package.*` which likely isn't what users configuring their application intend.
By stripping the period here, we retain backwards compatibility for excluded package lists that still have trailing periods.
However, we do drop support for wildcard package exclusions where applications didn't include a trailing period and genuinely desired the previous behaviour. They will need to move such exclusions to the regex exclusion list instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] sonarcloud[bot] commented on pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #736:
URL: https://github.com/apache/struts/pull/736#issuecomment-1686456196
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=736)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=736&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=736&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=736&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=736&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_coverage&view=list) [91.4% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=736&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #736: WW-5337 Make SecurityMemberAccess exclusion checking more performant
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #736:
URL: https://github.com/apache/struts/pull/736#discussion_r1300194447
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -261,7 +262,8 @@ public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses)
}
private Set<String> parseExcludedPackageNames(String commaDelimitedPackageNames) {
- Set<String> parsedSet = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames);
+ Set<String> parsedSet = commaDelimitedStringToSet(commaDelimitedPackageNames)
+ .stream().map(s -> strip(s, ".")).collect(toSet());
Review Comment:
I think it's unintuitive to require a period at the end of each excluded package. Currently if the period is missed, the exclusion will act like the regex `com\.package.*` which likely isn't what users configuring their application intend.
By stripping the period here, we retain backwards compatibility for excluded package lists that still have trailing periods.
However, we do drop support for wildcard package exclusions where applications didn't include a trailing period and genuinely desired the previous behaviour. They will need to move such exclusions to the regex exclusion list instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org