More strict parsing of pageable parameters [DATACMNS-1696]

[spring-projects-issues]

Paul Vorbach opened DATACMNS-1696 and commented

During a penetration test, we found that you can enter pretty much anything to the query parameters of endpoints that take an argument of type Pageable and the value will get accepted.

For instance, if you have a @RestController listening on /myPageableResource, you can perform requests to /myPageableResource?page=0%20AND%201%3D1%20-- (for readability the URL-decoded value of the page parameter is "0 AND 1=1 --"), the request will be accepted and the default value of 0 for page will be used. The same holds true at least for the ignoreCase parameter of the sort parameters.

I'd argue that it would be better to return a 400 Bad Request instead in this case, where the value for page is obviously invalid. If users happen to enter a bad value by accident, they might not realize why they always get the first page as a result.

 Suggestions for the implementation:

The pageable parameters get parsed by the PageableHandlerMethodArgumentResolver. There also is a PageableHandlerMethodArgumentResolverCustomizer, which can be used to e.g. change parameter names or the maximum page size. Since changing the parsing behavior would be breaking, I'd suggest to allow for customization via the PageableHandlerMethodArgumentResolverCustomizer instead, but a breaking change could probably also be acceptable here.

Do you think this is a valid request? Are there other ways to customize the current behaviour that I have missed (other than re-implementing the HandlerMethodArgumentResolver and changing its behavior)?

---

Affects: 2.1.16 (Lovelace SR16)

[spring-projects-issues]

Oliver Drotbohm commented

Thanks for reporting this. We follow the guideline to use the default (global or what's declared on the invoked controller method) as an implementation of Postel's law meaning we treat invalid values as if they were missing from the request.

You have a point that that approach is debatable and I can see an argument made that defaulting should apply if the value for a request parameter is missing, but if one is given, it needs to be semantically correct.

I think we can introduce a flag in PageableHandlerMethodArgumentResolver to explicitly reject values if they're malformed and allow users to enable that flag in their configuration through a PageableHandlerMethodArgumentResolverCustomizer and probably later downstream via a simple property in Spring Boot

[spring-projects-issues]

Oliver Drotbohm commented

Giving this a shoot here are some challenges we face trying to reject these values:

1. Do we abort the verification on the first offending value or do we attempt to parse all of them? The latter complicates the implementation quite a bit.
2. From a client point of view sorting and paging are one abstraction. I.e. if a user enabled this flag, would she expect invalid attributes piped into sort to be rejected, too? Currently the parsing of a Sort instance is not tied to a type at all and the additional handling, that e.g. Spring Data REST applies (to take Jackson customizations into account) also simply drop property paths not found on the target type. I.e. during the parsing, in a non-Spring Data REST context we do not know whether the value is invalid and can't really properly reject it

[spring-projects-issues]

Paul Vorbach commented

Right – defaulting to a value is perfectly fine if a parameter is missing.

The points in your second comment are both valid. I see how collecting all bad values and returning helpful error messages would be hard to do. I wouldn't expect and API to do this in every circumstance.

Regarding the second point, I think yes, a user would expect paging and sorting to be treated the same way. Maybe the gap that you simply don't know which properties are valid and which are not could be filled by providing an interface, which then can be implemented in user application code?