CXF-8619 Prevent double URL-decoding for form parameters where the pa…#878
CXF-8619 Prevent double URL-decoding for form parameters where the pa…#878reta merged 2 commits intoapache:3.4.x-fixesfrom
Conversation
…rameters are read from HttpServletRequest.getParameter()
| // Do not decode unless the key is empty value, fe @FormParam("") | ||
| FormUtils.populateMapFromStringOrHttpRequest(params, m, body, enc, StringUtils.isEmpty(key) && decode); | ||
|
|
||
| if (!StringUtils.isEmpty(key) && decode) { |
There was a problem hiding this comment.
I think this is change is not right: it applies only when if (params == null), which holds true for first parameter only. This is the reason why it was located at the end of the function. PS: the test cases are failing because of that.
There was a problem hiding this comment.
I see those test failures now, I'll take a look.
|
|
||
| List<String> values = Arrays.asList(parameterValues); | ||
|
|
||
| if (!decode) { |
There was a problem hiding this comment.
I think we should not encode the input. I understand that you are trying to mitigate the decoding issue(s), I am looking into the issue, will try to find out alternative solution.
There was a problem hiding this comment.
I'm not a fan of the solution, which is probably a slightly bizarre comment to make on my own code. There is a scenario where CXF is reading parameters from HttpServetReques.getParameter(), where the parameter itself is annotated with @Encoded. In that scenario, I'm not sure how we can provide the parameter correctly without re-encoding.
There was a problem hiding this comment.
not using getParameter but the body inputstream - think cxf already has some part of it in body readers? - can solves it.
another side note is that moving the evaluation to be lazy instead of eager in a list can also be a good thing and avoid a lot of preprocessing (think if (param1 is ok) { processBigParam(param2); }, if param1 is not ok you don't care of having processed param2 and worse if you have some security checks in your endpoint (@Context HttpServletRequest is not that rare ;)). This can be done in a custom MultivaluedMap impl (new RequestParametersMultivaluedMap<>(request)).
Hope it makes sense.
There was a problem hiding this comment.
not using getParameter but the body inputstream - think cxf already has some part of it in body readers? - can solves it.
Yes, the body inputstream is handled right before
|
Thank you for the contribution, @jgallimore, I have the concerns regarding this change, looking into the issue in order to understand the decoding flaws. |
|
Thank you so much for the very quick review, @reta. I'm very happy to work with you and the team to come to the right solution on this. Please feel free to nudge me in the right direction :). On thing I am wondering is whether the systest does a good job of highlighting the issue. I can certainly add some tests with an embedded Tomcat if that is useful. |
|
Thanks @jgallimore
I think it does, we continuously enrich it with new test cases and the fact it captured the regression is a good sign |
|
@jgallimore I think I understood the problem and I have a very simple solution to suggest, would you mind if I push the changes to your PR so you could take a look and provide the feedback? Thank you. |
|
@reta Yes please, that would be fantastic, thank you! |
|
@jgallimore done, please let me know what do you think, thank you! |
|
@jgallimore thank you
At least for me, the uncertainty here is that we either "trust" the request params as being properly decoded or not. If we do, that basically means @encoded is noop in this flow - we don't know if the request params were encoded in first place or not - the servlet hides this from us and just provides the final value. Contrary, if we get rid of this "trust" logic (basically, drop this pull request), then by adding |
|
From my perspective, I'd keep the PR as-is. If there's a need for the original encoded data, then its best to not consume the InputStream - instead leave it alone for CXF to parse. Is this good to merge in, or does it need anything else? I'm very happy to help in any way. |
|
@jgallimore thank you, sounds reasonable
I would like to run JAX-RS TCK tests over this PR, it is a bit sensitive change and it is better to make sure we don't introduce new failures, will update you shortly |
…come from ServletRequest request parameters
|
@jgallimore just finished the JAX-RS TCK run, no new test case failures, I think we are good to go once build checks are done, thank you! |
#878) * CXF-8619 Prevent double URL-decoding for form parameters where the parameters are read from HttpServletRequest.getParameter() * Introduce contextual property to mark form params as encoded if they come from ServletRequest request parameters Co-authored-by: Andriy Redko <drreta@gmail.com>
#878) * CXF-8619 Prevent double URL-decoding for form parameters where the parameters are read from HttpServletRequest.getParameter() * Introduce contextual property to mark form params as encoded if they come from ServletRequest request parameters Co-authored-by: Andriy Redko <drreta@gmail.com>
3 participants
…rameters are read from HttpServletRequest.getParameter()