[
https://issues.apache.org/jira/browse/WW-5829?focusedWorklogId=1052411&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1052411
]
ASF GitHub Bot logged work on WW-5829:
--------------------------------------
Author: ASF GitHub Bot
Created on: 04/Nov/26 11:42
Start Date: 04/Nov/26 11:42
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #2087:
URL: https://github.com/apache/struts/pull/2087#discussion_r4466981806
##########
core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java:
##########
@@ -1133,6 +1134,57 @@ public boolean acceptableParameterValue(String
parameterValue) {
assertEquals(expected, actual);
}
+ public void
testExcludedValuePatternsApplyToEveryValueOfMultiValuedParameter() throws
Exception {
+ ParametersInterceptor pi = createParametersInterceptor();
+ pi.setExcludedValuePatterns(".*\\$\\{.*?\\}.*");
+
+ final Map<String, Object> actual = injectValueStackFactory(pi);
+ ValueStack stack = injectValueStack(actual);
+
+ Map<String, Object> parameters = new HashMap<>();
+ parameters.put("single", new String[]{"fooValue"});
+ parameters.put("allGood", new String[]{"fooValue", "barValue"});
+ parameters.put("firstBad", new String[]{"${2+2}", "fooValue"});
+ parameters.put("secondBad", new String[]{"fooValue", "${2+2}"});
+ pi.applyParameters(new NoParametersAction(), stack,
HttpParameters.create(parameters).build());
+
+ assertEquals(Set.of("single", "allGood"), actual.keySet());
Review Comment:
All three tests assert only `keySet()`. A later change that accepts
`allGood` but binds only its first value would still pass them. Could you also
assert the bound value, e.g. `assertArrayEquals(new String[]{"fooValue",
"barValue"}, (String[]) actual.get("allGood"));`, in each of the three tests?
##########
core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java:
##########
@@ -1133,6 +1134,57 @@ public boolean acceptableParameterValue(String
parameterValue) {
assertEquals(expected, actual);
}
+ public void
testExcludedValuePatternsApplyToEveryValueOfMultiValuedParameter() throws
Exception {
+ ParametersInterceptor pi = createParametersInterceptor();
+ pi.setExcludedValuePatterns(".*\\$\\{.*?\\}.*");
+
+ final Map<String, Object> actual = injectValueStackFactory(pi);
+ ValueStack stack = injectValueStack(actual);
+
+ Map<String, Object> parameters = new HashMap<>();
+ parameters.put("single", new String[]{"fooValue"});
+ parameters.put("allGood", new String[]{"fooValue", "barValue"});
+ parameters.put("firstBad", new String[]{"${2+2}", "fooValue"});
+ parameters.put("secondBad", new String[]{"fooValue", "${2+2}"});
+ pi.applyParameters(new NoParametersAction(), stack,
HttpParameters.create(parameters).build());
+
+ assertEquals(Set.of("single", "allGood"), actual.keySet());
+ }
+
+ public void
testAcceptedValuePatternsApplyToEveryValueOfMultiValuedParameter() throws
Exception {
+ ParametersInterceptor pi = createParametersInterceptor();
+ pi.setAcceptedValuePatterns("^fooValue.*");
+
+ final Map<String, Object> actual = injectValueStackFactory(pi);
+ ValueStack stack = injectValueStack(actual);
+
+ Map<String, Object> parameters = new HashMap<>();
+ parameters.put("single", new String[]{"fooValue"});
+ parameters.put("allGood", new String[]{"fooValue", "fooValue2"});
+ parameters.put("secondBad", new String[]{"fooValue", "barValue"});
Review Comment:
The PR description says each test covers single / all good / first bad /
second bad, but this one has no `firstBad` case. Could you add e.g.
`parameters.put("firstBad", new String[]{"barValue", "fooValue"});`, so a
version that skips index 0 or checks only the last value also fails here?
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -549,11 +549,17 @@ protected BeanInfo getBeanInfo(Object action) {
* @return true if parameter is accepted
*/
protected boolean isAcceptableParameterValue(Parameter param, Object
action) {
- return isAcceptableParameterValueAware(param, action) &&
isAcceptableValue(param.getName(), param.getValue());
+ return isAcceptableParameterValueAware(param, action)
+ && Arrays.stream(valuesToCheck(param)).allMatch(value ->
isAcceptableValue(param.getName(), value));
Review Comment:
Optional: `valuesToCheck` runs twice per parameter, and each run rebuilds
the array several times through `isMultiple()` (via `isDefined()`) and
`getMultipleValues()`. That's about six `toStringArray()` calls per
multi-valued parameter. Computing the array once here and passing it to both
checks would avoid that.
Also optional: with no value patterns configured (the default), each value
still goes through `isParamValueExcluded`/`isParamValueAccepted`, and each logs
a "not defined" DEBUG line. A large checkbox list now produces two lines per
value. An early return when neither pattern set is configured would keep that
to the old one pass.
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -549,11 +549,17 @@ protected BeanInfo getBeanInfo(Object action) {
* @return true if parameter is accepted
*/
protected boolean isAcceptableParameterValue(Parameter param, Object
action) {
- return isAcceptableParameterValueAware(param, action) &&
isAcceptableValue(param.getName(), param.getValue());
+ return isAcceptableParameterValueAware(param, action)
+ && Arrays.stream(valuesToCheck(param)).allMatch(value ->
isAcceptableValue(param.getName(), value));
}
protected boolean isAcceptableParameterValueAware(Parameter param, Object
action) {
- return !(action instanceof ParameterValueAware valueAware) ||
valueAware.acceptableParameterValue(param.getValue());
+ return !(action instanceof ParameterValueAware valueAware)
+ ||
Arrays.stream(valuesToCheck(param)).allMatch(valueAware::acceptableParameterValue);
+ }
+
+ private String[] valuesToCheck(Parameter param) {
Review Comment:
Optional: since `isAcceptableParameterValue` /
`isAcceptableParameterValueAware` are protected extension points, making this
helper `protected` too would let a subclass that overrides one of them reuse
the same expansion instead of falling back to `param.getValue()`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1052411)
Time Spent: 20m (was: 10m)
> ParametersInterceptor value filters only check the first value of a repeated
> parameter
> --------------------------------------------------------------------------------------
>
> Key: WW-5829
> URL: https://issues.apache.org/jira/browse/WW-5829
> Project: Struts 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 6.12.0, 7.0.0, 7.3.0, 7.4.0
> Reporter: Gouri Sankar A
> Priority: Minor
> Fix For: 7.5.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> ParametersInterceptor checks a parameter's value with
> ParameterValueAware.acceptableParameterValue(...) and with
> excludedValuePatterns / acceptedValuePatterns, but passes only
> Parameter.getValue(), which for a repeated parameter is the first value. The
> whole String[] is then set on the action.
> Reproduction (main, against a running Struts Boot app): an action
> implementing ParameterValueAware that rejects "evil", with a String[] setter
> annotated @StrutsParameter.
> ?tags=evil -> rejected
> ?tags=evil&tags=ok -> rejected
> ?tags=ok&tags=evil -> accepted, the action receives [ok, evil]
> The same holds for excludedValuePatterns and acceptedValuePatterns.
> The ParameterValueAware javadoc says it lets an action reject values "to
> prevent clients from setting other unexpected (and possibly dangerous)
> parameter values". JSONInterceptor already checks every element of a JSON
> array (WW-4858).
> Affected versions: the same check (param.getValue(), which is the first
> value) is present in the 6.12.0, 7.0.0, 7.3.0 and 7.4.0 tags, by source
> inspection. The behaviour itself was only reproduced on main.
> Fix: check every value of a repeated parameter. One rejected value rejects
> the whole parameter, as a rejected first value does today. Single-valued and
> empty parameters are checked as before.
> Tests: three new tests in ParametersInterceptorTest (excluded patterns,
> accepted patterns, ParameterValueAware). They fail without the change and
> pass with it.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
