You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by "kusalk (via GitHub)" <gi...@apache.org> on 2023/10/05 17:19:14 UTC
[PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
kusalk opened a new pull request, #760:
URL: https://github.com/apache/struts/pull/760
WW-5340
--
Quick follow-up to https://github.com/apache/struts/pull/747
See comments
--
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: commits-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #760:
URL: https://github.com/apache/struts/pull/760#discussion_r1348236030
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -142,26 +142,22 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
}
long size = item.getSize();
- if (size == 0) {
- values.add(StringUtils.EMPTY);
- } else if (size > maxStringLength) {
+ if (size > maxStringLength) {
+ LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", item.getFieldName(), size, maxStringLength);
Review Comment:
I assume so, yet this is a user controlled value so maybe it needs escaping
--
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
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk merged PR #760:
URL: https://github.com/apache/struts/pull/760
--
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: commits-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #760:
URL: https://github.com/apache/struts/pull/760#issuecomment-1749350634
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=760)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=VULNERABILITY) [1 Vulnerability](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=760&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=760&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=760&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_coverage&view=list) [87.9% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_duplicated_lines_density&view=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-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #760:
URL: https://github.com/apache/struts/pull/760#discussion_r1347756216
##########
core/src/main/java/org/apache/struts2/ognl/StrutsOgnlGuard.java:
##########
@@ -71,28 +71,38 @@ public boolean isRawExpressionBlocked(String expr) {
@Override
public boolean isParsedTreeBlocked(Object tree) {
- return containsExcludedNodeType(tree);
+ if (!(tree instanceof Node) || skipTreeCheck((Node) tree)) {
+ return false;
+ }
+ return recurseNodes((Node) tree);
}
- protected boolean containsExcludedNodeType(Object tree) {
- if (!(tree instanceof Node) || excludedNodeTypes.isEmpty()) {
- return false;
+ protected boolean skipTreeCheck(Node tree) {
+ return excludedNodeTypes.isEmpty();
+ }
+
+ protected boolean recurseNodes(Node node) {
+ if (checkNode(node)) {
+ return true;
+ }
+ for (int i = 0; i < node.jjtGetNumChildren(); i++) {
+ if (recurseNodes(node.jjtGetChild(i))) {
+ return true;
+ }
}
- return recurseExcludedNodeType((Node) tree);
+ return false;
+ }
+
+ protected boolean checkNode(Node node) {
Review Comment:
I separated the recursion logic from the node checking logic so that subclasses don't need to unnecessarily duplicate that code when overriding.
--
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
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #760:
URL: https://github.com/apache/struts/pull/760#discussion_r1347752551
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -142,26 +142,22 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
}
long size = item.getSize();
- if (size == 0) {
- values.add(StringUtils.EMPTY);
- } else if (size > maxStringLength) {
+ if (size > maxStringLength) {
+ LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", item.getFieldName(), size, maxStringLength);
Review Comment:
Just added this line, no other functional changes here
--
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
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #760:
URL: https://github.com/apache/struts/pull/760#discussion_r1348243095
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -142,26 +142,22 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
}
long size = item.getSize();
- if (size == 0) {
- values.add(StringUtils.EMPTY);
- } else if (size > maxStringLength) {
+ if (size > maxStringLength) {
+ LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", item.getFieldName(), size, maxStringLength);
Review Comment:
Not sure a field name can even contain newline characters but anyway SonarCloud is happy now
--
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
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #760:
URL: https://github.com/apache/struts/pull/760#discussion_r1347763341
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -142,26 +142,22 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
}
long size = item.getSize();
- if (size == 0) {
- values.add(StringUtils.EMPTY);
- } else if (size > maxStringLength) {
+ if (size > maxStringLength) {
+ LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", item.getFieldName(), size, maxStringLength);
Review Comment:
I think logging the field name at the debug level is fine in terms of security?
--
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
Re: [PR] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #760:
URL: https://github.com/apache/struts/pull/760#issuecomment-1749983243
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=760)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=760&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=760&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=760&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=760&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_coverage&view=list) [86.1% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=760&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