In _update_fetch_data() (update.fetch.inc) there's no validation done to ensure that 'update_max_fetch_time' is numeric, so if someone was hacking around with different variable values they could break that function when it tries to add e.g. a string to the output of time().

CommentFileSizeAuthor
#1 drupal-n1092000.patch1.23 KBdamienmckenna

Comments

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Needs review
StatusFileSize
new drupal-n1092000.patch1.23 KB

Patch attached that ensures only numeric values are added to time().

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

I looked at update.test to see where tests could be added, any suggestions on how to test this? Maybe setting a string or object value on update_max_fetch_time and testing that it doesn't give an error?

bfroehle’s picture

bfroehle commented
Status: Needs review » Needs work

I'm do not think this level of babysitting is really necessary. The Drupal way seems to be to do validation on entering items into the database, but no validation when you retrieve them. If you go mess around with the variables and set one to an improper value, well that's your own fault. So generally I'm against this patch.

However, if this is destined for inclusion, I think using is_numeric() would be more appropriate than is_object() as proposed in #2.

bfroehle’s picture

bfroehle commented

The inclusion of a page to change the setting like in #1091992: Provide UI element to modify 'update_max_fetch_time' variable would have the validation there anyway.

bfroehle’s picture

bfroehle commented
Status: Needs work » Closed (won't fix)

Nowhere else in Drupal do we do this level of babysitting. As far as I'm concerned, manually changing variables falls into the you break it, you buy it category.

For a large discussion relating to this variable, see #952394: "No available releases found" - fetching update information exceeds timeout.