Ordering of WebMvcMetricsFilter breaks character encoding

[vpavic]

Actuator's WebMvcMetricsFilter has a hard-coded order of Ordered.HIGHEST_PRECEDENCE, which in practice puts it ahead of CharacterEncodingFilter. Since WebMvcMetricsFilter performs the mapping introspection, under some circumstances it will touch the POST data hence breaking the CharacterEncodingFilter.

I've managed to put together a sample app that exhibits this behavior with a specific set of request mapping paths. After starting the app, issue the following HTTP POST request (samples use HTTPie):

$ http -f POST :8080/pv/post name='Vedran Pavić' Content-Type:'application/x-www-form-urlencoded'
HTTP/1.1 200 
Content-Length: 21
Content-Type: text/plain;charset=UTF-8
Date: Thu, 11 Jan 2018 17:58:44 GMT

Hello Vedran Pavi�

When spring-boot-starter-actuator is removed from the dependencies, which gets WebMvcMetricsFilter out of the picture, request is processed as expected:

$ http -f POST :8080/pv/post name='Vedran Pavić' Content-Type:'application/x-www-form-urlencoded'
HTTP/1.1 200 
Content-Length: 19
Content-Type: text/plain;charset=UTF-8
Date: Thu, 11 Jan 2018 17:59:32 GMT

Hello Vedran Pavić

[vpavic]

On top of addressing the ordering problem that leads to this issue, would it makes sense to introduce some sort of validation that checks whether CharacterEncodingFilter is the first filter in the chain, and log a warning if it isn't?

Boot could have a test that verifies this, since it has happened a few time that other filters got in front of CharacterEncodingFilter, and also the users could be warned if they incidentally add a filter that gets ordered in front.

[snicoll]

ErrorPageFilter is also flagged Ordered.HIGHEST_PRECEDENCE

[sriki77]

Should the CharacterEncodingFilter be made to implement PriorityOrdered with highest precedence?

[sriki77]

Making OrderedCharacterEncodingFilter implement PriorityOrdered makes it work. I will try and prepare a pull request, if this is acceptable solution.

[snicoll]

@sriki77 of course. Particularly interested by a regression test for this.

[sriki77]

I spent some time analysing the the issue. The reason is when WebMvcMetricsFilter triggers parameter parsing. This happens when there parameter based request mapping exists.

So the possible work around for if anybody facing right away is...

@GetMapping(params = { "first", "second" })
public String getWithParams() {
 return "getWithParams";
}

@PostMapping(path = "/{pv}/post")
public String handlePost(@PathVariable("pv") String pv, Form form) {
return "Hello " + form.getName();
}

Remove @GetMapping which use parameter based mapping, then post will work fine or remove the path variable from post then it will work fine.

@snicoll I have simulated the effect in integration test; pull request coming soon.

[vpavic]

Remove @GetMapping which use parameter based mapping, then post will work fine or remove the path variable from post then it will work fine.

I wouldn't call this a workaround as it requires you to drop the handler mapping that's perfectly valid. The actual workaround that's viable would involve, as you initially suggested, extending the CharacterEncodingFilter to implement PriorityOrdered and replace the OrderedCharacterEncodingFilter provided by Boot.

That being said, I'm not sure Boot should make implementing PriorityOrdered a permanent fix as it sounds a bit extreme measure for a web filter (are there any web filters implementing PriorityOrdered across the Spring projects?). Additionally, if for some reason users really do want to get something in front of CharacterEncodingFilter that would be a potential problem for them. And like with the current situation, if in the future there is some other framework provided filter that also implements PriorityOrdered with the same order we'd have the same situation (that's why IMO web filter implementing PriorityOrdered is a dangerous precedent).

IMO Boot should consider having a well defined (and tested) order of web filters it provides/configures which would make it easier both for Boot itself and users to add new filters to the mix as the results would be predictable.

[sriki77]

@vpavic Absolutely sorry on my miscommunication in wording. I always meant OrderedCharacterEncodingFilter not CharacterEncodingFilter. I wanted Spring boot to deal with the issue not Spring itself.The pull request i submitted 2 days ago makes OrderedCharacterEncodingFilter implement PriorityOrdered. I have also added a integration test that simulates the issue. Please do take a look and let me know your thoughts.

[bclozel]

I'm not in favor of using PriorityOrdered for the reasons Vedran @vpavic mentioned.

I don't think we can really consistently test filters and their ordering as we'd need to include all auto-configurations or risk missing something if we add a new filter somewhere. Also, some auto-configurations are excluding each other, so this is more complex than that.

Same thing for the startup check: I'm worried this would be either too late, or it could trigger early initialization of filters and lead to other issues.

The only idea I've got so far is reviewing the priorities and documenting them, especially for: OrderedCharacterEncodingFilter, ErrorPageFilter, WebMvcMetricsFilter, WebRequestTraceFilter, ApplicationContextHeaderFilter, HiddenHttpMethodFilter, HttpPutFormContentFilter.

[sriki77]

If i understand it right, the better option is to ensure OrderedCharacterEncodingFilter is before WebMvcMetricsFilter without making it priority ordered. I will trying and take a look. Any specific place i need to look or any additional info that will help?

[sriki77]

@bclozel if this is a reasonable default,
OrderedCharacterEncodingFilter, ErrorPageFilter, WebMvcMetricsFilter, WebRequestTraceFilter, ApplicationContextHeaderFilter, HiddenHttpMethodFilter, HttpPutFormContentFilter.

Let's use the FilterRegistrationBean.REQUEST_WRAPPER_FILTER_MAX_ORDER - x with appropriate values of x to ensure the above filter ordering is honoured.

Let me know your thoughts.

[sriki77]

@bclozel let me know if you need the test in the pull request, i can a separate on containing only the test.