Repository navigation
WW-5829 fix(interceptor): apply parameter value filters to every value of a multi-valued parameter - #2087
WW-5829 fix(interceptor): apply parameter value filters to every value of a multi-valued parameter#2087g0w6y wants to merge 2 commits into
Conversation
…e of a multi-valued parameter ParametersInterceptor passed only Parameter.getValue() (the first value) to ParameterValueAware and the excluded/accepted value patterns, so a request such as a=ok&a=evil was accepted although "evil" would have been rejected. Check each value. Unlike JSONInterceptor, which drops only the bad list element, a single rejected value rejects the whole parameter (as a rejected first value already did). Single-valued and empty parameters behave as before.
lukaszlenart
left a comment
lukaszlenart
left a comment
There was a problem hiding this comment.
Thanks, the change looks right to me. Every value that ends up bound is now checked, ActionMappingParametersInterceptor picks it up without changes, and single-valued and empty parameters behave as before. I ran ParametersInterceptorTest, ParameterAuthorizerTest, StrutsParameterAnnotationTest and ActionMappingParametersInterceptorTest, and all pass. With the ParametersInterceptor change reverted, exactly the three new tests fail.
Two test gaps to close before merging, plus three optional cleanups, as inline comments.
| 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"}); |
There was a problem hiding this 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?
| parameters.put("secondBad", new String[]{"fooValue", "${2+2}"}); | ||
| pi.applyParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build()); | ||
|
|
||
| assertEquals(Set.of("single", "allGood"), actual.keySet()); |
There was a problem hiding this 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?
| 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)); |
There was a problem hiding this 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.
| || Arrays.stream(valuesToCheck(param)).allMatch(valueAware::acceptableParameterValue); | ||
| } | ||
|
|
||
| private String[] valuesToCheck(Parameter param) { |
There was a problem hiding this 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().
…aluesToCheck Close the two required test gaps from review: - testAcceptedValuePatternsApplyToEveryValueOfMultiValuedParameter had no firstBad case, unlike the other two tests, so a version that skipped index 0 wouldn't have been caught here. - All three multi-value tests asserted only actual.keySet(), so a version that accepted a parameter but bound only its first value would still have passed. Add assertArrayEquals on the bound value in each. Also widen valuesToCheck from private to protected, so a subclass overriding isAcceptableParameterValue/isAcceptableParameterValueAware can reuse the same expansion instead of falling back to param.getValue().
|
Thanks for the detailed review. Closed both required gaps (16b0e2a):
Also took the Left the double-computation optimization as is: avoiding it cleanly means either bypassing virtual dispatch on Full core suite: 3628/0 failures. |
Fixes WW-5829
What & why
ParametersInterceptorhands onlyParameter.getValue()(the first value) toParameterValueAware.acceptableParameterValueand to the excluded/accepted value patterns, but then sets the wholeString[]on the action. So?tags=ok&tags=evilpasses a filter that rejectsevil, while?tags=eviland?tags=evil&tags=okare rejected.JSONInterceptoralready checks every element of a JSON array (WW-4858). This checks every value of a repeated form parameter too, with one difference: JSONInterceptor drops only the bad list element, while here one rejected value rejects the whole parameter.Change
isAcceptableParameterValueandisAcceptableParameterValueAwarenow check every value of a multi-valued parameter (small private helpervaluesToCheck). One rejected value rejects the whole parameter, which is what already happens when the first value is rejected. Single-valued and empty parameters go through the same calls with the same argument as before.How verified
Three tests added to
ParametersInterceptorTest(excluded patterns, accepted patterns,ParameterValueAware), each covering single value, all values good, first value bad, second value bad.Tests run: 48, Failures: 3(all three new tests:secondBadreached the stack).ParametersInterceptorTest(48),ParameterAuthorizerTest(42) andStrutsParameterAnnotationTest(33) pass.ParameterValueAwarethat rejectsevil:?tags=ok&tags=evilwas accepted before (the action received[ok, evil]) and is rejected with the change;?tags=ok&tags=finestill binds.Not changed
Risk
Behaviour changes only for actions that configure value patterns or implement
ParameterValueAwareand receive repeated parameters where a later value is rejected; those requests were previously (wrongly) accepted. Cost is one extra pattern match per additional value.