Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixes an issue when creating a date select with too many options. #239

Closed
wants to merge 1 commit into from

7 participants

@dlt

This commit fixes issue #6627 (https://rails.lighthouseapp.com/projects/8994/tickets/6627-server-hanging-when-using-extreme-values-for-date_select-start_year). The issue occurs because the build_options method loops forever when the range between start_date and end_date is too big.

This commit adds a verification to #build_options and raises an error when the range is greater than 1000.

@sikachu sikachu commented on the diff
actionpack/lib/action_view/helpers/date_helper.rb
@@ -810,6 +810,12 @@ module ActionView
step = options.delete(:step) || 1
options.reverse_merge!({:leading_zeros => true})
leading_zeros = options.delete(:leading_zeros)
+ limit = (stop - start).abs
+ max_options = 1000
+
+ if limit / step.abs > max_options
+ raise ArgumentError, "Cannot build too many options for select."
+ end
@sikachu Collaborator
sikachu added a note

I feel like the limit variable name should be more clear, something like number_of_options. Also, max_options can surely be renamed and make it as constant somewhere. I think that should be better.

Also, I would love if the exception message become more clearer too. Something like "There're too many number of years option to be built. Are you sure you aren't mistyped something?" would be more friendly.

Thank you so much for patching this edge case. Please update your patch and force push to your branch, so it will stay as single concise commit. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sikachu
Collaborator

Oh, an also if you can update the CHANGELOG for the new limitation that would be awesome.

@dost

I think hard limit 1000 is bad for any application with historic dates. Maybe including max limit as optional parameter would help in such situations.

@andmej

@sikachu, what would be the appropriate place to add the max_options limit?

@dost making the limit optional wouldn't fix the issue at all.

@henrikhodne

@andmej I think he meant optional parameter as in you don't have to supply it, and if you don't it defaults to, say, 1000.

@libo

Since there hasn't been so much action on this issue I wrote my patch, hope it's fine.
libo@631eca1

@jeremy jeremy closed this pull request from a commit
Liborio Cannici Fixes an issue when creating a date select with too many options.
Inspired by dlt dlt@9e61563

Closes #239.

Conflicts:

	actionpack/CHANGELOG
114218e
@jeremy jeremy closed this in 114218e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
6 actionpack/lib/action_view/helpers/date_helper.rb
@@ -810,6 +810,12 @@ def build_options(selected, options = {})
step = options.delete(:step) || 1
options.reverse_merge!({:leading_zeros => true})
leading_zeros = options.delete(:leading_zeros)
+ limit = (stop - start).abs
+ max_options = 1000
+
+ if limit / step.abs > max_options
+ raise ArgumentError, "Cannot build too many options for select."
+ end
@sikachu Collaborator
sikachu added a note

I feel like the limit variable name should be more clear, something like number_of_options. Also, max_options can surely be renamed and make it as constant somewhere. I think that should be better.

Also, I would love if the exception message become more clearer too. Something like "There're too many number of years option to be built. Are you sure you aren't mistyped something?" would be more friendly.

Thank you so much for patching this edge case. Please update your patch and force push to your branch, so it will stay as single concise commit. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
select_options = []
start.step(stop, step) do |i|
View
5 actionpack/test/template/date_helper_test.rb
@@ -668,6 +668,11 @@ def test_select_date_with_order
assert_dom_equal expected, select_date(Time.mktime(2003, 8, 16), :start_year => 2003, :end_year => 2005, :prefix => "date[first]", :order => [:month, :day, :year])
end
+ def test_select_date_with_too_big_range_between_start_year_and_end_year
+ assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), :start_year => 2000, :end_year => 20000.years, :prefix => "date[first]", :order => [:month, :day, :year]) }
+ assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), :start_year => Date.today.year - 100.years, :end_year => 2000, :prefix => "date[first]", :order => [:month, :day, :year]) }
+ end
+
def test_select_date_with_incomplete_order
# Since the order is incomplete nothing will be shown
expected = %(<input id="date_first_year" name="date[first][year]" type="hidden" value="2003" />\n)
Something went wrong with that request. Please try again.