You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2021/01/21 11:14:06 UTC
[GitHub] [struts] gregh3269 opened a new pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
gregh3269 opened a new pull request #469:
URL: https://github.com/apache/struts/pull/469
There are unnecessary log warning when DMI is enabled, from the ParametersInterceptor.
WARN com.opensymphony.xwork2.interceptor.ParametersInterceptor ParametersInterceptor:isAccepted - Parameter [action:myAction!save] didn't match accepted pattern [[\w+((\.\w+)|(\[\d+])|((\d+))|(['(\w|[\u4e00-\u9fa5])'])|(('(\w|[\u4e00-\u9fa5])')))*]]! See Accepted / Excluded patterns at https://struts.apache.org/security/#accepted--excluded-patterns
eg the property 'action:myAction!save' should not be considered as a bean/property parameter, as its used as part of DMI to submit the form.
Any property which matches the DMI method invocation "^(action|method):.*" needs to be silently ignored and not logged in devMode=true.
DMI_AWARE_ACCEPTED_PATTERNS can also be dropped from DefaultAcceptedPatternsChecker as the DMI action|method would never be a form property.
----------------------------------------------------------------
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.
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 closed pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
Posted by GitBox <gi...@apache.org>.
lukaszlenart closed pull request #469:
URL: https://github.com/apache/struts/pull/469
--
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 change in pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #469:
URL: https://github.com/apache/struts/pull/469#discussion_r563171653
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
##########
@@ -324,12 +324,12 @@ protected boolean isAccepted(String paramName) {
protected boolean isExcluded(String paramName) {
ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
if (result.isExcluded()) {
- if (devMode) { // warn only when in devMode
+ if (devMode && result.isLog()) { // warn only when in devMode and required
Review comment:
What's the reason of ignoring some patterns and some not in logging?
##########
File path: core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
##########
@@ -84,6 +85,14 @@ protected void setDynamicMethodInvocation(String dmiValue) {
if (!BooleanUtils.toBoolean(dmiValue)) {
LOG.debug("DMI is disabled, adding DMI related excluded patterns");
setAdditionalExcludePatterns("^(action|method):.*");
+ } else {
+ LOG.debug("DMI is enabled, adding DMI related ignored patterns");
+ ignoredPatterns = new HashSet<>();
+ try {
+ ignoredPatterns.add(Pattern.compile("^(action|method):.*", Pattern.CASE_INSENSITIVE));
+ } finally {
+ ignoredPatterns = Collections.unmodifiableSet(ignoredPatterns);
+ }
Review comment:
Basically right now it doesn't matter if DMI is enabled or not, `action|method` will be excluded. Wouldn't be better to just extend the base RegEx? Or setup this pattern in constructor?
```java
public DefaultExcludedPatternsChecker() {
setExcludedPatterns(EXCLUDED_PATTERNS);
setAdditionalExcludePatterns("^(action|method):.*");
}
```
So then additional `ignoredPatterns` field is not needed.
##########
File path: core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java
##########
@@ -66,18 +66,24 @@
private final boolean excluded;
private final String excludedPattern;
+ private final boolean log;
Review comment:
I'm not a fun of such flags as they are quite misleading and control something out of scope of this class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls commented on pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #469:
URL: https://github.com/apache/struts/pull/469#issuecomment-869131624
[](https://coveralls.io/builds/40910804)
Coverage remained the same at 47.311% when pulling **3dcbe6be33a927e1208233506a6142b8726cff11 on gregh3269:struts-2-5-x** into **90ea4d8aa1ca1ecc488137deab968e1bc873bdcd on apache:struts-2-5-x**.
--
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] yasserzamani commented on pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #469:
URL: https://github.com/apache/struts/pull/469#issuecomment-825549392
I think your DMI isn't enabled at all because I see that DefaultAcceptedPatternsChecker setAcceptedPatterns(DMI_AWARE_ACCEPTED_PATTERNS) when DMI is enabled but in same time I see that in this PR description, the logged accepted pattern isn't DMI_AWARE_ACCEPTED_PATTERNS. It is ACCEPTED_PATTERNS which starts with \w+((\., provided DMI_AWARE_ACCEPTED_PATTERNS starts with \w+([:].
Otherwise (if it's enabled really) it should work as per tested [testDmiIsEnabled](https://github.com/apache/struts/blob/09f969a9bebe31370df64702a61420f14ead6271/core/src/test/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsCheckerTest.java#L205).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #469:
URL: https://github.com/apache/struts/pull/469#issuecomment-992206856
I'm closing this PR as it changes nothing.
--
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] lukaszlenart commented on a change in pull request #469: WW-5115 Reduce logging for DMI excluded parameters.
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #469:
URL: https://github.com/apache/struts/pull/469#discussion_r563171483
##########
File path: core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java
##########
@@ -66,18 +66,24 @@
private final boolean excluded;
private final String excludedPattern;
+ private final boolean log;
Review comment:
I'm not a fan of such flags as they are quite misleading and control something out of scope of this class.
--
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