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/04/08 19:37:05 UTC
[GitHub] [struts] yasserzamani opened a new pull request #483: fix double evaluations...
yasserzamani opened a new pull request #483:
URL: https://github.com/apache/struts/pull/483
* fix UIBean and its subclasses double evaluations
* fix Param tag used with Bean tag double evaluations
(to be continued)
Reference:
https://securitylab.github.com/research/apache-struts-double-evaluation/
--
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] coveralls edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38867748)
Coverage increased (+0.04%) to 49.953% when pulling **b9c4ec0dc3318149f9279659c58b0ac3cb347cff on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-971807722
@lukaszlenart I think so, yes please
--
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] coveralls edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/41190590)
Coverage increased (+0.1%) to 50.042% when pulling **9a3ebf8d19b4b64e4b61c9d2a20bfaa070457c2c on fix/double_evaluations** into **dd807e78a8768c390feff8bd554b6faa4202eeca on master**.
--
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 a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r751478225
##########
File path: core/src/main/java/com/opensymphony/xwork2/security/NotExcludedAcceptedPatternsChecker.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.opensymphony.xwork2.security;
+
+/**
+ * Used across different places to check if given string is not excluded and is accepted
+ * @see <a href="https://securitylab.github.com/research/apache-struts-double-evaluation/">here</a>
+ * @since 2.5.27
Review comment:
thanks! fixed
--
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] coveralls edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/39223422)
Coverage increased (+0.1%) to 50.043% when pulling **466fcfbfa2476a8c643288648bce002afa09ca3b on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-830788154
Thanks a lot! Clever catches :) I'll reform as suggested :+1:
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r676652228
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -796,28 +792,31 @@ public void evaluateParams() {
// see if the value was specified as a parameter already
+ final String NAME_VALUE = "nameValue";
Review comment:
I'm not sure, because only this method needs it - Again it's recommended by SonarLint to not repeat "nameValue" more than three times but only this method needs this constant!
--
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] coveralls edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38710148)
Coverage increased (+0.02%) to 49.932% when pulling **a6e95685b8ee9a1657a921f975f7d6931486f14b on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r676647916
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -589,73 +586,72 @@ protected void mergeTemplate(Writer writer, Template template) throws Exception
}
public String getTemplateDir() {
- String templateDir = null;
+ String result = null;
Review comment:
Recommended by SonarLint to keep local variable names different than the class fields names.
--
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] aleksandr-m commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
aleksandr-m commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r677556555
##########
File path: core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java
##########
@@ -418,6 +419,8 @@ public void register(ContainerBuilder builder, LocatableProperties props) {
/** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.PROTOTYPE **/
alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
alias(AcceptedPatternsChecker.class, StrutsConstants.STRUTS_ACCEPTED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
+ alias(NotExcludedAcceptedPatternsChecker.class, StrutsConstants.STRUTS_NOT_EXCLUDED_ACCEPTED_PATTERNS_CHECKER
+ , builder, props, Scope.SINGLETON);
Review comment:
I think it is about comma in the begging of the line.
--
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 a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615238766
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/AliasInterceptor.java
##########
@@ -167,7 +182,13 @@ public void setAliasesKey(String aliasesKey) {
for (Object o : aliases.entrySet()) {
Map.Entry entry = (Map.Entry) o;
String name = entry.getKey().toString();
+ if (isNotAcceptableExpression(name)) {
+ continue;
+ }
String alias = (String) entry.getValue();
+ if (isNotAcceptableExpression(alias)) {
+ continue;
+ }
Review comment:
BTW I knew and understand that you probably mean that those are developer constant expressions and I shouldn't consider the first evaluation actually a real evaluation at this specific case for example, but unfortunately here in this interceptor I don't have an easy way to recognize if it's really evaluated and has parsed variables or not, in Component.java I had it, `!parsedResult.equals(findString(name))`. So unfortunately I can't for now easily distinguish between `#{'application.myName':'myName'}` and `#{ #parameters['name'] : #parameters['alias'] }` where in first one evaluation is just a constant to String but in second one it's reading from end user parameters!
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-840614051
Let's leave it as is, no better idea so your is good enough :)
--
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] JCgH4164838Gh792C124B5 commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-830911008
Hi @yasserzamani . This looks like an interesting PR and discussion for sure. 😃
One other consideration may be allowing a choice (by configuration) as to whether to allow re-evaluation or not (at all).
If a developer wants to permit re-evaluation then they could enable a configuration option to do so, otherwise the framework could continue to apply the more restrictive original logic (https://github.com/apache/struts/pull/483/files#diff-cfe644a2b24b492d6835fa1f38e7a770dad354b286cbe6b056a5fe7e80e669caL808-L809).
Also, having an opt-in choice might mitigate some of the potential concerns with additional overhead, since the developer would be making a choice to use the feature.
In terms of having the ability to configure different enter/interceptor accept/reject patterns vs. tag/component accept/reject patterns, there might be hard-to-predict scenarios where one might need some flexibility to change them independently. Having the tag/component accept/reject patterns default to the same as the enter/interceptor ones, but with a user-configurable override, might be a safer path to take ?
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-830781310
I have a large concern with using `*PatternsChecker`s here, they supposed to be used as a _border pass control_ - validating if [incoming parameters](https://struts.apache.org/security/#accepted--excluded-patterns) are allowed or not. Now they have been used internally to validate expression evaluation which can be hard to understand by users. Also there is no possibility to have more restrictive patterns on _enter_ (in interceptors) and looser patterns internally. Another thing is that those `*PatternsChecker`s are created per interceptor, the same will happen here, but having a large number of tags will consume a lot of memory instating new `*PatternsChecker`s per each tag.
Having that said I would introduce a dedicated Pattern Checker instance specifically used in tags, with simple one pattern used to verify expression evaluation. It also has to be a Singleton to reduce memory consumption.
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38681471)
Coverage increased (+0.008%) to 49.918% when pulling **a7884f9df923fa1e07222d131ab0a413bdd13823 on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615233956
##########
File path: core/src/main/java/org/apache/struts2/components/Component.java
##########
@@ -571,4 +576,39 @@ public boolean isValidTagAttribute(String attrName) {
return standardAttributes;
}
+ protected boolean isAccepted(String paramName) {
+ AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName);
+ if (result.isAccepted()) {
+ return true;
+ }
+
+ LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getAcceptedPattern());
+
+ return false;
+ }
+
+ protected boolean isExcluded(String paramName) {
+ ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
+ if (!result.isExcluded()) {
+ return false;
+ }
+
+ LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getExcludedPattern());
+
+ return true;
+ }
Review comment:
Thanks for your review! Yes those interceptors probably already have validated them but here, I revalidate some of them them before reevaluating because some of them need to be reevaluated (e.g. name, alias etc) so I revalidate them before, if they're going really to be double evaluated. For example, if name is `getName()` expression then I don't validate it because our translateVariable actually doesn't evaluate it and return it as is, but when name is `array[%{fooIndex}]` then I validate its parsed result if it's going to go for second evaluation. For instance if its parsed result is `array[1]` then it passes my validation here but if it's `array[1-1]` Struts doesn't allow this anymore :)
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r676648362
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -589,73 +586,72 @@ protected void mergeTemplate(Writer writer, Template template) throws Exception
}
public String getTemplateDir() {
- String templateDir = null;
+ String result = null;
if (this.templateDir != null) {
- templateDir = findString(this.templateDir);
+ result = findString(this.templateDir);
}
// If templateDir is not explicitly given,
// try to find attribute which states the dir set to use
- if (StringUtils.isBlank(templateDir)) {
- templateDir = stack.findString("#attr.templateDir");
+ if (StringUtils.isBlank(result)) {
+ result = stack.findString("#attr.templateDir");
}
// Default template set
- if (StringUtils.isBlank(templateDir)) {
- templateDir = defaultTemplateDir;
+ if (StringUtils.isBlank(result)) {
+ result = defaultTemplateDir;
}
// Defaults to 'template'
- if (StringUtils.isBlank(templateDir)) {
- templateDir = "template";
+ if (StringUtils.isBlank(result)) {
+ result = "template";
}
- return templateDir;
+ return result;
}
public String getTheme() {
- String theme = null;
+ String result = null;
Review comment:
Recommended by SonarLint to keep local variable names different than the class fields names.
--
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] aleksandr-m commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
aleksandr-m commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r677556555
##########
File path: core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java
##########
@@ -418,6 +419,8 @@ public void register(ContainerBuilder builder, LocatableProperties props) {
/** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.PROTOTYPE **/
alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
alias(AcceptedPatternsChecker.class, StrutsConstants.STRUTS_ACCEPTED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
+ alias(NotExcludedAcceptedPatternsChecker.class, StrutsConstants.STRUTS_NOT_EXCLUDED_ACCEPTED_PATTERNS_CHECKER
+ , builder, props, Scope.SINGLETON);
Review comment:
I believe it is about comma in the begging of the line.
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615232225
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/AliasInterceptor.java
##########
@@ -167,7 +182,13 @@ public void setAliasesKey(String aliasesKey) {
for (Object o : aliases.entrySet()) {
Map.Entry entry = (Map.Entry) o;
String name = entry.getKey().toString();
+ if (isNotAcceptableExpression(name)) {
+ continue;
+ }
String alias = (String) entry.getValue();
+ if (isNotAcceptableExpression(alias)) {
+ continue;
+ }
Review comment:
Why did add this restrictions? User can define an alias `#{'application.myName':'myName'}` and this is a valid scenario, with your changes it won't work probably. Aso why do you want restrict internal expressions?
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615236192
##########
File path: core/src/main/java/org/apache/struts2/components/Component.java
##########
@@ -571,4 +576,39 @@ public boolean isValidTagAttribute(String attrName) {
return standardAttributes;
}
+ protected boolean isAccepted(String paramName) {
+ AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName);
+ if (result.isAccepted()) {
+ return true;
+ }
+
+ LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getAcceptedPattern());
+
+ return false;
+ }
+
+ protected boolean isExcluded(String paramName) {
+ ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
+ if (!result.isExcluded()) {
+ return false;
+ }
+
+ LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getExcludedPattern());
+
+ return true;
+ }
Review comment:
That being said if I want to summarize, the new policy is: A parsed result of `translateVariable` method must also be valid if it's going to go for second evaluation.
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-827349385
Thanks for your review @aleksandr-m ! Yes I think it will work. Assume `<s:sometag someAttr="someComplexExp"...`. I don't disturb the evaluation of `someComplexExp` at all. It will be evaluated as before. But once evaluated for first time, if it's going to go for another evaluation for second time, then I check and validate it again against accepted/excluded patterns. I know it might limit developer in some special not-usual usages but I have to do so if I want to reduce the risk of end-user raw data evaluation by mistake. Furthermore developer always can change her/his design if this breaks it.
For example, I think in your case, in first evaluation we will have e.g. `id->itemId2` and `key->items[2].name` where both will pass accepted/excluded patterns if they are needed to be re-evaluated.
I'll add corresponding test:+1:thanks!
--
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 edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/39428745)
Coverage increased (+0.1%) to 50.042% when pulling **a37e35be6133f64b459d47b5429484f6532b3ffa on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-831193561
Hi @JCgH4164838Gh792C124B5 thanks! :)
Actually it's not a feature requested, it's a security mitigation. Its main purpose is to mitigate when developer is using %{} or ${} against end user not validated input by mistake. It prevents it only in suspicious cases because reevaluation is naturally needed in Struts and cannot be opt-out.
Moving on your second paragraph, I personally think it's not a new thing. It's a "complement" for current already exist accept/exclude policy. I think it's same and current policy had merely missed this piece. My reasoning: Consider `<s:text name="foo[%{index}].name"/>`. It's just same as `<s:text name="foo[0].name"/><s:text name="foo[1].name"/><s:text name="foo[2].name"/><s:text name="foo[3].name"/>...`. My inference: To validate that if it's ok to double evaluate `foo[%{index}].name` we just need to validate if the result of the first evaluation is already considered as a valid input! Do you know what I mean? I feel they aren't different, the second evaluation is same as the first evaluation regarding validation. e.g. if in some places we had third evaluation, then we could say the result of the second evaluation also must be an already considered as valid before going to third validation and so on.
So I think it's not a new concept, it merely was a missing piece in previous impl. The only issue here is the memory. Frankly what I've never understood yet in Struts is if e.g. OGNL exclusions or Interceptor exclusions are for security so why developer is allowed to override them?! because otherwise I could change current impl to singleton and solve the mem issue:)
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615233956
##########
File path: core/src/main/java/org/apache/struts2/components/Component.java
##########
@@ -571,4 +576,39 @@ public boolean isValidTagAttribute(String attrName) {
return standardAttributes;
}
+ protected boolean isAccepted(String paramName) {
+ AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName);
+ if (result.isAccepted()) {
+ return true;
+ }
+
+ LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getAcceptedPattern());
+
+ return false;
+ }
+
+ protected boolean isExcluded(String paramName) {
+ ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
+ if (!result.isExcluded()) {
+ return false;
+ }
+
+ LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getExcludedPattern());
+
+ return true;
+ }
Review comment:
Thanks for your review! Yes those interceptors probably already have validated them but here, I revalidate some of them before reevaluating because some of them need to be reevaluated (e.g. name, alias etc) so I revalidate them before, if they're going really to be double evaluated. For example, if name is `getName()` expression then I don't validate it because our translateVariable actually doesn't evaluate it and return it as is, but when name is `array[%{fooIndex}]` then I validate its parsed result if it's going to go for second evaluation. For instance if its parsed result is `array[1]` then it passes my validation here but if it's `array[1-1]` Struts doesn't allow this anymore :)
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r676655020
##########
File path: core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java
##########
@@ -418,6 +419,8 @@ public void register(ContainerBuilder builder, LocatableProperties props) {
/** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.PROTOTYPE **/
alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
alias(AcceptedPatternsChecker.class, StrutsConstants.STRUTS_ACCEPTED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
+ alias(NotExcludedAcceptedPatternsChecker.class, StrutsConstants.STRUTS_NOT_EXCLUDED_ACCEPTED_PATTERNS_CHECKER
+ , builder, props, Scope.SINGLETON);
Review comment:
IntelliJ has a vertical line indicating suggested line length limit. I always press enter on it to not violate it. Do you want me to keep all in same line? I can if so :)
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-830840632
@lukaszlenart the thing is they're not completely decoupled. They're dependent to each other. For example currently by default `getFoo().name` is not accepted (should be changed to `foo.name` to get accepted) but if developer wants to add it to the accepted patterns then it must implicitly be also accepted for re-evaluations. That being said, even they must be equal. This has no meaning to restrict a pattern to be evaluated again if developer wants to allow, vice versa, no meaning to allow a pattern to be re-evaluated if developer hasn't explicitly allowed it. Evaluation patterns and re-evaluation patterns are naturally same. wdyt?
I think the only remaining issue is memory. wdyt? May I simply define singletons from default accept/exclude patterns classes and use them instead in tags?
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-876219597
---
**NOTE**
BTW please use `Squash and merge`.
---
--
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] coveralls edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/44327959)
Coverage increased (+0.04%) to 49.955% when pulling **ba6a2b5510238a31c05a56370f3d87a04e02d1f1 on fix/double_evaluations** into **dd807e78a8768c390feff8bd554b6faa4202eeca on master**.
--
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 pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-832190412
@yasserzamani you can use the same default patterns in both cases but I would give user a chance to change them independently. You cannot turn a bean into Singleton programmatically, the scope is defined in [`struts-default.xml`](https://github.com/apache/struts/blob/master/core/src/main/resources/struts-default.xml#L224-L225) as `prototype` so each time a new instance will be created. This is fine for interceptors as they are created per stack, each interceptor is a _Singleton_ for a given interceptors stack. This won't work for tags, as tags are re-created per each request and each tag is a separated instance.
--
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] yasserzamani merged pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani merged pull request #483:
URL: https://github.com/apache/struts/pull/483
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-836243359
Yes I see. I'd named it `ReevaluationPatternsChecker` but then I thought what about if in future it will be required to being reused for another purpose rather than reevaluation. If it makes sense I can rename it :+1:
--
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 a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615232488
##########
File path: core/src/main/java/org/apache/struts2/components/Component.java
##########
@@ -571,4 +576,39 @@ public boolean isValidTagAttribute(String attrName) {
return standardAttributes;
}
+ protected boolean isAccepted(String paramName) {
+ AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName);
+ if (result.isAccepted()) {
+ return true;
+ }
+
+ LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getAcceptedPattern());
+
+ return false;
+ }
+
+ protected boolean isExcluded(String paramName) {
+ ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
+ if (!result.isExcluded()) {
+ return false;
+ }
+
+ LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getExcludedPattern());
+
+ return true;
+ }
Review comment:
Why do you perform this check again on the component level? Those parameters were already validated in `ParametersInterceptor` or `CookieInterceptor`
--
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 edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38907453)
Coverage increased (+0.1%) to 50.038% when pulling **921ba1d472197b6d4b00852e1f03aa2c15555ca2 on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-833813537
> you can use the same default patterns in both cases but I would give user a chance to change them independently.
:+1: thanks! @lukaszlenart I think I did so in last commit now. Could you please review?
--
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 a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r675993450
##########
File path: core/src/main/java/com/opensymphony/xwork2/security/NotExcludedAcceptedPatternsChecker.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.opensymphony.xwork2.security;
+
+/**
+ * Used across different places to check if given string is not excluded and is accepted
+ * @see <a href="https://securitylab.github.com/research/apache-struts-double-evaluation/">here</a>
+ * @since 2.5.27
Review comment:
As I already pointed out, I wouldn't introduce this change in 2.5.27
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -796,28 +792,31 @@ public void evaluateParams() {
// see if the value was specified as a parameter already
+ final String NAME_VALUE = "nameValue";
Review comment:
Shouldn't this be class level constant with protected scope?
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -589,73 +586,72 @@ protected void mergeTemplate(Writer writer, Template template) throws Exception
}
public String getTemplateDir() {
- String templateDir = null;
+ String result = null;
if (this.templateDir != null) {
- templateDir = findString(this.templateDir);
+ result = findString(this.templateDir);
}
// If templateDir is not explicitly given,
// try to find attribute which states the dir set to use
- if (StringUtils.isBlank(templateDir)) {
- templateDir = stack.findString("#attr.templateDir");
+ if (StringUtils.isBlank(result)) {
+ result = stack.findString("#attr.templateDir");
}
// Default template set
- if (StringUtils.isBlank(templateDir)) {
- templateDir = defaultTemplateDir;
+ if (StringUtils.isBlank(result)) {
+ result = defaultTemplateDir;
}
// Defaults to 'template'
- if (StringUtils.isBlank(templateDir)) {
- templateDir = "template";
+ if (StringUtils.isBlank(result)) {
+ result = "template";
}
- return templateDir;
+ return result;
}
public String getTheme() {
- String theme = null;
+ String result = null;
Review comment:
Why did you rename this local variable?
##########
File path: core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java
##########
@@ -212,6 +214,8 @@ public void register(ContainerBuilder builder, LocatableProperties props)
.factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class, Scope.PROTOTYPE)
.factory(AcceptedPatternsChecker.class, DefaultAcceptedPatternsChecker.class, Scope.PROTOTYPE)
+ .factory(NotExcludedAcceptedPatternsChecker.class, DefaultNotExcludedAcceptedPatternsChecker.class
+ , Scope.SINGLETON)
Review comment:
Strange formatting
##########
File path: core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java
##########
@@ -418,6 +419,8 @@ public void register(ContainerBuilder builder, LocatableProperties props) {
/** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.PROTOTYPE **/
alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
alias(AcceptedPatternsChecker.class, StrutsConstants.STRUTS_ACCEPTED_PATTERNS_CHECKER, builder, props, Scope.PROTOTYPE);
+ alias(NotExcludedAcceptedPatternsChecker.class, StrutsConstants.STRUTS_NOT_EXCLUDED_ACCEPTED_PATTERNS_CHECKER
+ , builder, props, Scope.SINGLETON);
Review comment:
Strange formatting
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -589,73 +586,72 @@ protected void mergeTemplate(Writer writer, Template template) throws Exception
}
public String getTemplateDir() {
- String templateDir = null;
+ String result = null;
Review comment:
Why did you rename this local variable?
--
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] coveralls edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/39860952)
Coverage increased (+0.1%) to 50.039% when pulling **3a62e32f737be8296e8f3340687b918e584ed5da on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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 edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38903689)
Coverage increased (+0.05%) to 49.956% when pulling **cd95114043ecb11eea908ad48bd0008265154aaf on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-970059506
I think this ready to be `Squash & merge`d
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-829165726
@aleksandr-m as promised, [here you are](https://github.com/apache/struts/blob/466fcfbfa2476a8c643288648bce002afa09ca3b/core/src/test/java/org/apache/struts2/views/freemarker/FreemarkerResultMockedTest.java#L278). It addresses your case right?
--
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 #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-836219620
The name `NotExcludedAcceptedPatternsChecker` is a bit awkward but looks good, I will let others take a look before merging this change, thanks Yasser!
--
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] aleksandr-m commented on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
aleksandr-m commented on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-827142363
@yasserzamani Will this work with the following code?
```
<s:iterator value="..." status="stat">
<s:textfield id="itemId%{#stat.index}" key="items[%{#stat.index}].name" />
</s:iterator>
```
Can we have some tests for that too?
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r623122804
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/AliasInterceptor.java
##########
@@ -167,7 +182,13 @@ public void setAliasesKey(String aliasesKey) {
for (Object o : aliases.entrySet()) {
Map.Entry entry = (Map.Entry) o;
String name = entry.getKey().toString();
+ if (isNotAcceptableExpression(name)) {
+ continue;
+ }
String alias = (String) entry.getValue();
+ if (isNotAcceptableExpression(alias)) {
+ continue;
+ }
Review comment:
@lukaszlenart would you like resolve conversation if it addresses your concern? thanks again for your review!
--
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 edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38823243)
Coverage increased (+0.04%) to 49.948% when pulling **16904445db7fd9d9c7422152559af4a78e15dad3 on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r623122567
##########
File path: core/src/main/java/org/apache/struts2/components/Component.java
##########
@@ -571,4 +576,39 @@ public boolean isValidTagAttribute(String attrName) {
return standardAttributes;
}
+ protected boolean isAccepted(String paramName) {
+ AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName);
+ if (result.isAccepted()) {
+ return true;
+ }
+
+ LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getAcceptedPattern());
+
+ return false;
+ }
+
+ protected boolean isExcluded(String paramName) {
+ ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
+ if (!result.isExcluded()) {
+ return false;
+ }
+
+ LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getExcludedPattern());
+
+ return true;
+ }
Review comment:
@lukaszlenart would you like resolve conversation if it addresses your concern? thanks again for your review!
--
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] yasserzamani commented on a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r676646412
##########
File path: core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java
##########
@@ -212,6 +214,8 @@ public void register(ContainerBuilder builder, LocatableProperties props)
.factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class, Scope.PROTOTYPE)
.factory(AcceptedPatternsChecker.class, DefaultAcceptedPatternsChecker.class, Scope.PROTOTYPE)
+ .factory(NotExcludedAcceptedPatternsChecker.class, DefaultNotExcludedAcceptedPatternsChecker.class
+ , Scope.SINGLETON)
Review comment:
IntelliJ has a vertical line indicating suggested line length limit. I always press enter on it to not violate it. Do you want me to keep all in same line? I can if so :)
--
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 a change in pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #483:
URL: https://github.com/apache/struts/pull/483#discussion_r615237515
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/AliasInterceptor.java
##########
@@ -167,7 +182,13 @@ public void setAliasesKey(String aliasesKey) {
for (Object o : aliases.entrySet()) {
Map.Entry entry = (Map.Entry) o;
String name = entry.getKey().toString();
+ if (isNotAcceptableExpression(name)) {
+ continue;
+ }
String alias = (String) entry.getValue();
+ if (isNotAcceptableExpression(alias)) {
+ continue;
+ }
Review comment:
I think it will work. On first evaluation (where I don't disturb), the result will be same as always, a java map with key `application.myName` and value `myName`. On second evaluation (here that I've added above codes), I validate the both key and value are valid and they're.
I think this answers the second part, right? I don't restrict the whole expression, I restrict key and value in the result Map individually to be valid because they're going to go for another evaluation.
--
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 edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/38698460)
Coverage increased (+0.01%) to 49.923% when pulling **3fed2406e7ac21f5aee23443e09be46d07d3075a on fix/double_evaluations** into **09f969a9bebe31370df64702a61420f14ead6271 on master**.
--
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 edited a comment on pull request #483: fix double evaluations...
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #483:
URL: https://github.com/apache/struts/pull/483#issuecomment-816754380
[](https://coveralls.io/builds/41102561)
Coverage increased (+0.1%) to 50.039% when pulling **56836c408c3de3802a9304c65abb9ad66aa6fed5 on fix/double_evaluations** into **dd807e78a8768c390feff8bd554b6faa4202eeca on master**.
--
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