Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
sonarcloud[bot] commented on PR #780: URL: https://github.com/apache/struts/pull/780#issuecomment-1807061451 SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts=780) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [76.2% Coverage](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list)  Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome) -- 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-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
kusalk commented on PR #780: URL: https://github.com/apache/struts/pull/780#issuecomment-1807061357 @lukaszlenart I'm going to make it extensible as part of WW-5343 so that applications can add even stronger checks if they so desire - so will leave it not `final` -- 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-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
sonarcloud[bot] commented on PR #780: URL: https://github.com/apache/struts/pull/780#issuecomment-1793706015 SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts=780) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [76.2% Coverage](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list)  Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome) -- 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-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
sonarcloud[bot] commented on PR #780: URL: https://github.com/apache/struts/pull/780#issuecomment-1793701515 SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts=780) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [76.0% Coverage](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list)  Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome) -- 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-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
sonarcloud[bot] commented on PR #780: URL: https://github.com/apache/struts/pull/780#issuecomment-1793698505 SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts=780) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [75.2% Coverage](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list)  Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome) -- 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-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
kusalk commented on code in PR #780:
URL: https://github.com/apache/struts/pull/780#discussion_r1382547971
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,168 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
-if (!memberClass.isAssignableFrom(targetClass)) {
-throw new IllegalArgumentException("Target does not match
member!");
+if (target != null) {
+// Special case: Target is a Class object but not Class.class
+if (Class.class.equals(target.getClass()) &&
!Class.class.equals(target)) {
Review Comment:
Validate and throw exceptions if arguments are not what we expect as the
following logic depends on these assumptions to be accurate
--
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-unsubscr...@struts.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
kusalk commented on code in PR #780:
URL: https://github.com/apache/struts/pull/780#discussion_r1382538846
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
-if (!memberClass.isAssignableFrom(targetClass)) {
-throw new IllegalArgumentException("Target does not match
member!");
+if (target instanceof Class) { // Target may be of type Class for
static members
+if (!member.getDeclaringClass().equals(target)) {
+throw new IllegalArgumentException("Target class does not
match static member!");
Review Comment:
Throw an exception here because if this ceases to be the case, it means the
following logic may need changes
--
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-unsubscr...@struts.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
kusalk commented on code in PR #780:
URL: https://github.com/apache/struts/pull/780#discussion_r1382540169
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
-if (!memberClass.isAssignableFrom(targetClass)) {
-throw new IllegalArgumentException("Target does not match
member!");
+if (target instanceof Class) { // Target may be of type Class for
static members
+if (!member.getDeclaringClass().equals(target)) {
+throw new IllegalArgumentException("Target class does not
match static member!");
+}
+target = null;
Review Comment:
Set to null as there is no more useful information to extract here, and it
simplifies the checks/logic to follow
--
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-unsubscr...@struts.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
kusalk commented on code in PR #780:
URL: https://github.com/apache/struts/pull/780#discussion_r1382538745
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
Review Comment:
Wanted to be a bit more deliberate about how we handle the arguments in case
OGNL behaviour changes
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
-if (!memberClass.isAssignableFrom(targetClass)) {
-throw new IllegalArgumentException("Target does not match
member!");
+if (target instanceof Class) { // Target may be of type Class for
static members
+if (!member.getDeclaringClass().equals(target)) {
+throw new IllegalArgumentException("Target class does not
match static member!");
Review Comment:
Throw an exception here because if this ceases to be the case, it means the
following logic may need changes
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
-if (!memberClass.isAssignableFrom(targetClass)) {
-throw new IllegalArgumentException("Target does not match
member!");
+if (target instanceof Class) { // Target may be of type Class for
static members
+if (!member.getDeclaringClass().equals(target)) {
+throw new IllegalArgumentException("Target class does not
match static member!");
+}
+target = null;
+} else {
+if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+throw new IllegalArgumentException("Target does not match
member!");
+}
}
-if (disallowProxyMemberAccess && ProxyUtil.isProxyMember(member,
target)) {
Review Comment:
Extract this into its own protected method for consistency
##
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
-final int memberModifiers = member.getModifiers();
-final Class memberClass = member.getDeclaringClass();
-// target can be null in case of accessing static fields, since OGNL
3.2.8
-final Class targetClass = Modifier.isStatic(memberModifiers) ?
memberClass : target.getClass();
-if (!memberClass.isAssignableFrom(targetClass)) {
-throw new IllegalArgumentException("Target does not match
member!");
+if (target instanceof Class) { // Target may be of type Class for
static members
+if (!member.getDeclaringClass().equals(target)) {
+throw new IllegalArgumentException("Target class does not
match static member!");
+}
+target = null;
+} else {
+
Re: [PR] WW-5350 Refactor SecurityMemberAccess [struts]
sonarcloud[bot] commented on PR #780: URL: https://github.com/apache/struts/pull/780#issuecomment-1793685163 SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts=780) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts=780=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts=780=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts=780=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_struts=780=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [75.4% Coverage](https://sonarcloud.io/component_measures?id=apache_struts=780=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts=780=new_duplicated_lines_density=list)  Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome) -- 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-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
