The SearchApiFacetapiDate still processes the date range filter value when an invalid string is provided in the url. This issue is compounded when the invalid string starts with a number "2015something-that-is-not-validORbad".
Notice: A non well formed numeric value encountered in SearchApiFacetapiDate->createRangeFilter() (line 85 of /usr/share/webdrupal/html/sites/all/modules/search_api/contrib/search_api_facetapi/plugins/facetapi/query_type_date.inc).
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2665586-5--invalid_date_facet_values.patch | 3.18 KB | drunken monkey |
Comments
Comment #2
recrit commentedThe attached patch performs the following:
SearchApiFacetapiDate::parseRangeFilter- Parses the value into date parts and validates that each part is a positive integer. This is necessary since createRangeFilter() uses the DateTime::setDate and DateTime::setTime.SearchApiFacetapiDate::executeto skip addFacetFilter() if the created range filter is empty due to being invalid or no parts.Comment #3
drunken monkeyThanks a lot for creating this issue!
You're right, we currently don't really validate the user input there – mostly because users would have to manipulate the URLs manually to get wrong values in there, and because invalid data here doesn't really cause any grave problems or security concerns. But I guess being able to trigger errors is sometimes bad enough, and it can't do any harm to be thorough.
Anyways, while your patch already does look good, I think there are some improvements possible. Especially, the logic of rejecting any non-positive date parts is too restrictive – at least hours and minutes can definitely be 0. To be absolutely correct we'd have to also check that those values aren't too large, but otherwise, as your comment states, they automatically wrap anyways, so there shouldn't be any real problem there. The main concern here really is preventing non-integers from getting through.
Revised patch attached, please test/review!
Comment #4
recrit commented@drunken monkey: In patch #3's parseRangeFilter(), should
switch ($part)beswitch ($i)?Comment #5
drunken monkeyAh, yes, thanks for catching that!
Comment #6
recrit commentedlooks good, thanks!
Comment #7
drunken monkeyGood to hear, thanks for reviewing! And, of course, thanks again for the patch!
Committed.