FormHttpMessageConverter should throw more a more specific throwable when it encounters obviously malformed data.

[Helmsdown]

Let's say I have a Spring Boot service, that is picking up org.springframework.boot.autoconfigure.web.servlet.WebMvcAutoConfiguration. This means that by default I will get org.springframework.boot.web.servlet.filter.OrderedFormContentFilter installed. An arguably good default behavior. The implementation of the filter ends up using org.springframework.http.converter.FormHttpMessageConverter to parse the incoming request body. Good so far.

The implementation of org.springframework.http.converter.FormHttpMessageConverter#read leverages java.net.URLDecoder#decode(java.lang.String, java.nio.charset.Charset). Again why would you want to reimplement URL decoding functionality for no good reason?

Now imagine you work for a large engineering organization that has at least a thousand developers with 3+ thousand Spring Boot services. Each one has this filter installed doing an arguably good job of parsing HTTP form data.

But then one day, someone in the Security org is doing their job facilitating a pen-test across your entire fleet of services (many of which are Spring Boot services). The pen-testers are crafty folks. They like to contrive all sorts of naughty request payloads and URLs and try them on each and every one of your services. When they hit on a Spring Boot service, with a request payload that is obviously malicious URLDecoder.decode is going to do the following:

        while (((i + 2) < numChars) &&
               (c == '%')) {
            int v = Integer.parseInt(s, i + 1, i + 3, 16);
            if (v < 0)
                throw new IllegalArgumentException(
                        "URLDecoder: Illegal hex characters in escape "
                        + "(%) pattern - negative value");
            bytes[pos++] = (byte) v;
            i += 3;
            if (i < numChars)
                c = s.charAt(i);
        }

The good news is that the patently bad request has been rejected. The bad news is that IllegalArgumentException has been thrown, at a layer in your Spring Boot app that is likely before any custom exception handling is invoked. So the exception is going to ride on up to web server layer (e.g. Tomcat/Catalina) and get logged there.

Now imagine the company has spent a lot of resources building a system that tells your thousand+ developers when their apps are logging errors. The idea is that they can get notified when a novel error occurs in their system so they can clean it up. This is a great way to encourage error/logging hygiene over a large workforce of engineers. Unfortunately, on pen-test day, every developer with the error logging feature enabled got an email (per service they operate) that said:

Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception
java.lang.IllegalArgumentException: URLDecoder: Illegal hex characters in escape (%) pattern - Error at index 0 in: "@ "
  at java.net.URLDecoder.decode(URLDecoder.java:237)
  at org.springframework.http.converter.FormHttpMessageConverter.read(FormHttpMessageConverter.java:359)
  at org.springframework.web.filter.FormContentFilter.parseIfNecessary(FormContentFilter.java:109)
  at org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:88)
  at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
  at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164)
  at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140)
  at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
  at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164)
  at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140)

So naturally this facilitates a discussion between the various parties involved:

1. Service Owners who work on business specific problems in the company
2. The Security org tasked with ensuring an ever-improving security posture for the company
3. Platform folks who want to empower Service Owners to build maintainable, operable, and secure services

Everyone agrees that a pen-test should not increase cognitive load on the population of service owners. We don't want to own our variant of OrderedFormContentFilter as the Spring one does a good job. We really just want to filter this class of logging error out into a different bucket so our logging tooling doesn't email a thousand+ people. However, if we set up our logging infrastructure to filter out java.lang.IllegalArgumentException who knows what other class of problems we might hide and cause unintended damage.

So here's my request. Can we please have org.springframework.web.filter.FormContentFilter catch IllegalArgumentException and have it re-throw it as InvalidFormContentException (or some such exception)?

If you are amenable to this idea, I will submit the PR myself ASAP.

Thank you for your time.

[sdeleuze]

Would it work for your needs if org.springframework.http.converter.FormHttpMessageConverter#read was throwing an HttpMessageNotReadableException instead?

[Helmsdown]

Hmmmmm. Looks like the contract for org.springframework.http.converter.HttpMessageConverter already specifies that as a checked exception. The name is plausibly correct. I think that is a reasonable solution to this issue. Shall I submit a PR to that effect?

[sdeleuze]

If you want, sure. Please mention me in the PR, I will then close this issue as a duplicate, and review and merge it

[Helmsdown]

On it

[Helmsdown]

#34594