#4346 time_select not interpreting the multi-parameter correctly - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4346 open

time_select not interpreting the multi-parameter correctly

Reported by khagimoto | April 8th, 2010 @ 05:39 PM | in 3.0.6

Please see the post titled

"time_select and MultiparameterAssignmentErrors" on "Ruby on Rails: Talk" group here: http://groups.google.com/group/rubyonrails-talk/browse_thread/threa...

Gist is that when time_select helper is used with ANY options (:prompt => true, etc.), it fails to interpret the multi-parameter correctly.

Example:
When :ignore_date => true AND I don't include seconds, only params (4i) and (5i) are passed as expected, but they are interpreted as Year and Month instead of Hour and Minute.

Comments and changes to this ticket

You flagged this item as spam.
ultimasnake October 4th, 2010 @ 06:38 PM
At first I "blamed" an I8n compatiblity problem (wrong format interpeted) and was deeply looking into it, but khagimoto seems to be right. I have been using prompt for "please select" text for hours/minutes with the select_time and it kept giving me problems when selecting values above 12 minutes (also rather at random)... any value on :prompt different then true seems to break the code.
David Trasbo October 8th, 2010 @ 09:26 AM
- State changed from “new” to “incomplete”
- Importance changed from “” to “Low”
Please provide a patch with a fix/failing test.
Aditya Sanghi October 8th, 2010 @ 03:39 PM
I confirm this is a bug. I'm working on a failing test and a deeper description of the problem(s). Just a couple of hours ...
Aditya Sanghi October 8th, 2010 @ 08:51 PM
If you use time_select with the :ignore_date => true option, you will get this error. The patch i've added adds the failing test. The error received is "1 error(s) on assignment of multiparameter attributes". You will NOT get an error if the chosen minute is between 0-12. time select needs a default date, without the date, the Hour, Minute, Second become Year, Month, Day. So the Hour shifts to year, minute shifts to month (so if you have it bigger than 12 you get an error and if its less than 12 you don't get an error), seconds shifts to day (if you seconds greater than 30/31 you will also see an error).

Simply put, i think the documentation and the time_select function needs to be modified to not accept :ignore_date => true as an option.

Please see test "test_time_select_without_date_hidden_fields" in actionpack/test/template/date_helper_test.rb which tests for creation of fields without the date bits.

Once we are convinced about this. I'll submit another patch which will really fix the documentation, code, test etc.
- time_select_failing_test.diff 1.5 KB
Aditya Sanghi October 8th, 2010 @ 08:58 PM
- Assigned user set to “Santiago Pastorino”
Santiago Pastorino October 8th, 2010 @ 10:22 PM
Aditya can you provide a patch please?.
And this is for 3.0 I guess right?
Aditya Sanghi October 9th, 2010 @ 08:53 AM
I think this is a problem in 2.3.X also and yes I will provide a patch for that too.
Aditya Sanghi October 9th, 2010 @ 09:02 AM
Can someone highlight the use case for ":ignore_date => true" when using time_select by the way?

No point removing the option if there really is a valid use case for it.
Rohit Arondekar October 9th, 2010 @ 09:23 AM
This method will also generate 3 input hidden tags, for the actual year, month and day unless the option :ignore_date is set to true.

http://api.rubyonrails.org/classes/ActionView/Helpers/DateHelper.ht...
Aditya Sanghi October 9th, 2010 @ 09:42 AM
Thanks Rohit, I read the documentation but it doesn't say WHY we would need the :ignore_date field.

But the gumshoe that i am, the use-case for :ignore_date was found in this ticket more than 2 years ago.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

The requirement stated in this ticket is dubious to say the least but some how got approved. Emilio wants to use the time_select and date_select on the same field for design reasons. Why dont they just use datetime_select, i dont know?

I would love to hear from the core committers on their viewpoint before i proceed.

If nothing else gets fixed, at the very least the documentation should be update to ensure that if :ignore_date is used on time_select, a date_select on the same field is mandatory. Sounds silly to me anyway.
Rohit Arondekar October 9th, 2010 @ 10:01 AM
datetime_select will return a set of selects. What Emilio had asked for is the ability to have the same select tags in different locations. At least that's how I see it. I guess it's best if you wait for some direction on how to proceed here.

Anyways keep up the good work! :)
Santiago Pastorino October 9th, 2010 @ 04:42 PM
- Milestone set to “2.3.10”
- Assigned user cleared.
Aditya Sanghi October 9th, 2010 @ 04:57 PM
- State changed from “incomplete” to “open”
- Assigned user set to “Santiago Pastorino”
I'm including a comment update patch which should ideally be applied anyway. This remove redundant examples and invalid comment from above the time_select helper. I've also included a comment to ensure proper usage of :ignore_date => true (i.e. if you do this, you must also do date_select on the same attribute in the form).

@santiago, i request you to please have a look at the patch and commit if you think so.
- time_select_comments.diff 3 KB
Xavier Noria October 9th, 2010 @ 05:28 PM
- Assigned user changed from “Santiago Pastorino” to “Xavier Noria”
Xavier Noria October 10th, 2010 @ 10:12 AM
Aditya I think more coverage would be better for the user: a use case for :ignore_date, why is another helper needed at all, and why one wouldn't use a different helper in the first place instead of that pair.
Aditya Sanghi October 10th, 2010 @ 12:16 PM
Xavier, I'm actually at a total loss about the use case for this. Apparently it was introduced 2 years ago for design reasons in ticket #503.

Did you mean more coverage in tests or documentation?

Frankly, i believe the use case is invalid and ideally we should remove this functionality of :ignore_date => true.
Xavier Noria October 10th, 2010 @ 12:35 PM
Documentation coverage. This is an unclear option, in such cases it is good to explain why it is useful to the end-user. The motivation for #503 seems a good use-case. The explanation could briefly cover why the helper is generating hidden fields at all, which is the role of the date part in all this.
Aditya Sanghi October 10th, 2010 @ 01:26 PM
Xavier, why would one not use datetime_select instead for this? I've updated the documentation with more conviction this time :)
- time_select_comments2.diff 5.6 KB
Jeff Kreeftmeijer October 10th, 2010 @ 08:30 PM
- Tag changed from “errors, helpers, time_select” to “errors, helpers, patch, time_select”
Please use the "patch" tag when adding a patch to make sure patched tickets end up in the open patches bin. :)
You flagged this item as spam.
Andrea Campi October 13th, 2010 @ 07:58 AM
- Tag changed from “errors, helpers, patch, time_select” to “errors, helpers, patch, security, time_select”
Note this applies to edge too (just verified), so the milestone should be reset accordingly.

My first hunch was to find a fix for the root issue, as an alternative to documenting the current behavior (which is quite surprising IMO), but then I started delving deeper into the code, ActiveRecord::Base#assign_multiparameter_attributes in particular.

#assign_multiparameter_attributes calls @#execute_callstack_for_multiparameter_attributes@, and this method grew quite big over time; it does try to do the right thing (tm) for Dates, but that just ends messing things up for Time. I'd like to spend a bit more time on this and try a different solution; will provide a patch as soon as I'm done.

Just for early feedback, my current reasoning is this:
the current code trusts that all "multiparameter" values are provided, sequentially. In other words, these will be equivalent:
```
extract_callstack_for_multiparameter_attributes('datetime(1i)' => '1') # => {"sometime"=>[1]}
extract_callstack_for_multiparameter_attributes('datetime(1i)' => '', 'datetime(2i)' => '1') # => {"sometime"=>[nil, 1]}
# missing field
extract_callstack_for_multiparameter_attributes('datetime(2i)' => '1') # => {"sometime"=>[1]}
```
For a Date attribute, the nil are preserved and replaced with default values; for a Time attribute that doesn't happen, leading to POLA failures.
This looks silly to me: we have a position index, we should use it to either provide default values for missing parts (if sensible) or raise an exception if we still can't make sense of the input. What we should never, ever do is let malformed input silently cause unintended changes.

IN FACT, now that I think of it, I think this can be exploited by an attacker to change the date part of a Datetime, as long as she is authorized to change the time part of it:
```
t = TimeTest.new(:sometime => DateTime.now - 1.month)
 => #<TimeTest id: nil, sometime: "2010-09-13 06:50:55", created_at: nil, updated_at: nil> 
...

# legitimate access to a form with time_select(:sometime)
# params = {"time_test"=>{"sometime(1i)"=>"2010", "sometime(2i)"=>"9", "sometime(3i)"=>"13", "sometime(4i)"=>"06", "sometime(5i)"=>"12"}}
t.update_attributes(params['time_test']) # => true 
t                                        # => #<TimeTest id: 2, sometime: "2010-09-13 06:12:00", created_at: "2010-10-13 06:49:26", updated_at: "2010-09-13 2010-09-13 06:12:00"> 

# maliciously crafted POST
# params = {"time_test"=>{"sometime(4i)"=>"2009", "sometime(5i)"=>"12", "sometime(6i)"=>"12"}}
t.update_attributes(params['time_test']) # => true 
t                                        # => #<TimeTest id: 2, sometime: "2009-12-12 00:00:00", created_at: "2010-10-13 06:49:26", updated_at: "2010-10-13 06:51:06">
```
Because of this I think this issue should get a higher priority; I will provide a patch within the day.
You flagged this item as spam.
Andrea Campi October 13th, 2010 @ 08:08 AM
- Tag changed from “errors, helpers, patch, security, time_select” to “errors, helpers, patch, time_select”
(and just after I press update--sorry for following up on myself)

Maybe considering this a security issue is a bit of a stretch, as any web application trusting its input is bound to be broken way worse that this. But it would still make for a hell of a debugging session ;)
Aditya Sanghi October 13th, 2010 @ 03:31 PM
Awesome Andrea,

Actually i'd been working on the exactly the same thing as you but abandoned that line as i thought it might not be way to go.

Anyway, i'm attaching my patch (with tests) for review as well. This patch applies and runs on master branch though.
- multi_param_pola_fixes.diff 5 KB
You flagged this item as spam.
Andrea Campi October 13th, 2010 @ 04:32 PM
Aditya: here is my current patch (not in git format-patch yet), as you can see it's pretty similar in principle to yours :)

I want to go over it once more, because I realized there still is a failure case:
```
params = {"time_test"=>{"sometime(4i)"=>"2009", "sometime(6i)"=>"12"}}
```
I'm not sure this can happen using our helpers, but I'd rather be extra careful. My current thinking is to simply disallow missing pieces except in the last positions (of course empty ones would still be allowed).
- 4346.diff 4.5 KB
Aditya Sanghi October 13th, 2010 @ 08:39 PM
My preference is to give nils for missing components between 1 and the maximum provided key param to the class and let the class initializer handle or raise an error, except for date/time for which we default the date bits. For example, Time can handle a nil minute/hour.
You flagged this item as spam.
Andrea Campi October 13th, 2010 @ 10:47 PM
Good point. I'll give that I try.
Aditya Sanghi October 14th, 2010 @ 05:59 AM
Andrea, My patch takes care of filling in the missing bits with nil.
You flagged this item as spam.
Andrea Campi October 15th, 2010 @ 08:13 AM
Your patch makes sense if we want to implicitly accept a missing part of a multiparameter and interpret it just the same as if it where empty.

In other words, should these two cases behave the same?
```
  def test_multiparameter_attributes_on_time_with_raise_on_small_time_if_missing_date_parts
    attributes = {
      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
    }
    topic = Topic.find(1)
    topic.attributes = attributes
    assert_equal Time.local(1, 1, 1, 16, 12, 2), topic.written_on
  end


  def test_multiparameter_attributes_on_time_with_seconds_will_ignore_date_if_empty
    attributes = {
      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
    }
    topic = Topic.find(1)
    topic.attributes = attributes
    assert_equal Time.local(1, 1, 1, 16, 12, 02), topic.written_on
  end
```
Note that this is not specific to times and dates, the same can be argued of:
```
  def test_multiparameter_assignment_of_aggregation_with_missing_values
    customer = Customer.new
    address = Address.new("The Street", "The City", "The Country")
    attributes = { "address(2)" => address.city, "address(3)" => address.country }
    customer.attributes = attributes
    assert_equal Address.new(nil, "The City", "The Country"), customer.address
  end

  def test_multiparameter_assignment_of_aggregation_with_missing_values
    customer = Customer.new
    address = Address.new("The Street", "The City", "The Country")
    attributes = { "address(1)" => "", "address(2)" => address.city, "address(3)" => address.country }
    customer.attributes = attributes
    assert_equal Address.new(nil, "The City", "The Country"), customer.address
  end
```
I would say no, because in the latter the developer has explicitly told us "I know there is a Date portion of the attribute, and I want to overwrite it" whereas in the former we don't know for sure, it may be an oversight.

That's why my proposal is to instead raise if any part if missing, except at the end.

That said, since you've provided a patch I'm not going to object too strongly; I'll let someone else chime in.

However, I do have a few comments:
- defaulting to 2001 feels totally arbitrary, especially given that the current code for Date detaults to year 1;
- you definitely need more tests; I will attach mine (which your diff passes), and we may still need more;
- this method is already unreadable, it badly needs some refactoring; your code introduces a few new issues (please don't take this badly, nothing personal, I'm just known to be picky):
  
  there's no need to call to_a on a range before iterating, this is good enough: set_values = (1..values_with_empty_parameters.keys.max.to_i).collect do |position|
  
  you seem to have missed a couple of conversions from @values@ to @set_values@, in the @rescue@ blocks:
  
  in fact, @values@ looks unused now except for the initial if, I would rewrite it to be more efficient:
  
  if values_with_empty_parameters.values.any? { |v| !v.nil? } send(name + "=", nil) else
  
  adding a @#to_i@ to @find_parameter_position@ would let you remove the @#to_i@ / @#to_s@ when you fill @set_values@
I left this at the end because I couldn't think of a solution (in fact, it affected the patch I was working on, too), but with your patch it's quite trivial to perform a denial of service attack by making Ruby run out of memory:
```
attributes = { "address(100000000)" => "" } # add enough 0
```
That's a non-trivial problem that we would be introducing, and I think it needs to be addressed--if nothing else, by capping the position to 100 or so.

I'm attaching my additional tests (applied on top of your changes), see if they are any use and feel free to change them and integrate them in your final diff.
I'll be glad to review it and upvote it once the last concerns are addressed.

Edited by Rohit Arondekar for formatting.
- more_tests.diff 4 KB
Aditya Sanghi October 15th, 2010 @ 06:01 PM
Always humbled and glad that i'm doing more open source now. Your feedback is constructive, and i appreciate your attention to detail.
```
 Time.local(1,1,1,1,1,1) => Mon Jan 01 01:01:01 0530 2001
```
on my system. I think this depends on system architecture. I didn't look deep enough earlier and incorrectly defaulted to 2001.

I think the tests too have to be written so we're correctly checking for raised asserts, otherwise we'll get false positives.

Now this is how i've refactored it
- If its a Date parameter, missing values are defaulted to 1
- If its a Time parameter
  
  Error raised if NO date parameters were provided.
  
  If date parmeters were provided but were blank, they're defaulted to 1.
  
  If time parameters were not provided, they are defaulted to 0. (This is done because hour does not default to 0 if the year is 1, something quirky in how Time handles it and how DateTime.civil handles it).
- For other klasses
  
  If there are any missing parameters, an error is raised
  
  If the parameter is provided but is blank, it is passed to the initializer of that klass.
  
  A maximum of 100 parameters can be passed to the initializer.
Have another review of it Andrew. Thanks heaps.
- multi_param_pola_fixes_with_review_changes.diff 18.2 KB
You flagged this item as spam.
Andrea Campi October 16th, 2010 @ 08:13 PM
+1 to this version, thanks for cleaning it up :)

You should probably rebase the two commit together, just for clarity, but that's just me. Nice work regardless.
You flagged this item as spam.
Andrea Campi October 16th, 2010 @ 08:15 PM
- Tag changed from “errors, helpers, patch, time_select” to “errors, helpers, milestone:3.x, patch, time_select”
You flagged this item as spam.
Andrea Campi October 16th, 2010 @ 08:17 PM
- Tag changed from “errors, helpers, milestone:3.x, patch, time_select” to “errors, helpers, patch, time_select”
I'm not sure how to change the milestone, it's still set to 2.3.10 which is obviously incorrect since the patch is for edge.
Aditya Sanghi October 16th, 2010 @ 08:41 PM
- Milestone changed from “2.3.10” to “3.x”
- Importance changed from “Low” to “Medium”
Raising the priority from Low to Medium as it seems quite easy to manipulate dates and times using multiparameter attributes. Setting the milestone back to 3.x. But i think this patch can be applied to 2.3.x also, i believe as the code has not changed between 2.3.x and 3.x.

I'm still trying to upgrade my git-fu, so i'll update with a merged patch shortly when i learn to squash commits.
You flagged this item as spam.
Andrea Campi October 16th, 2010 @ 08:46 PM
Re: git, you can easily do that with (assuming these two are the only commits on this branch):
```
git rebase -i HEAD^^
```
and then changing the second commit from keep to @f@.

Re: milestone change, how did you do that? I thought adding a tag of milestone:3.x would do the trick (there's always something to learn :D ).
Aditya Sanghi October 16th, 2010 @ 08:56 PM
Adding rebased squashed patch.

(PS. I can change the milestone because i recently became a member of the Rails LH team. As pseudo-admins, there are extra input elements and edit links).
- 4346.diff 12.9 KB
You flagged this item as spam.
Andrea Campi October 16th, 2010 @ 09:24 PM
+1 from me :)
Aditya Sanghi October 17th, 2010 @ 07:47 PM
- Tag changed from “errors, helpers, patch, time_select” to “errors, helpers, multiparameter, multiparameter_attributes, patch, time_select”
Aditya Sanghi October 20th, 2010 @ 09:27 AM
- Milestone changed from “3.x” to “3.0.2”
Changing to 3.0.2 as it's a bugfix.
Aditya Sanghi October 23rd, 2010 @ 03:42 PM
@fxn, what do you think? would appreciate your views when you get a chance.
Aditya Sanghi October 29th, 2010 @ 07:58 AM
- Assigned user changed from “Xavier Noria” to “Santiago Pastorino”
Shameless bump for attention. :) I think this is important enough for someone from core to comment.
Xavier Noria October 29th, 2010 @ 08:34 AM
- Assigned user changed from “Santiago Pastorino” to “Xavier Noria”
Hey thanks for the ping.

I've not replied yet, but it is in my plate no worries.
Jeff Kreeftmeijer November 1st, 2010 @ 05:01 PM
- Tag cleared.
Automatic cleanup of spam.
Jeff Kreeftmeijer November 7th, 2010 @ 04:51 PM
Automatic cleanup of spam.
Jeff Kreeftmeijer November 8th, 2010 @ 08:28 AM
Automatic cleanup of spam.
Santiago Pastorino November 15th, 2010 @ 09:04 PM
- Milestone cleared.
[bulk edit]
Aditya Sanghi November 17th, 2010 @ 11:59 AM
Rebased against master (but secretly and gently bumping).

Forewarning: will bump again in 2 weeks if nothing happens in the meantime. :)
- 4346_master_new.diff 12.9 KB
You flagged this item as spam.
lakshmanan November 25th, 2010 @ 07:08 PM
Im using this time_select in my rails 3 project and I am facing the same problem.

Till the patch is committed to mainline, How do i fix it ?
You flagged this item as spam.
lakshmanan November 25th, 2010 @ 07:41 PM
Here is how I fixed it in my form for now. Hope this helps.

https://gist.github.com/715819
Aditya Sanghi November 26th, 2010 @ 09:21 AM
@lakshmanan your usage of :ignore_date => false seems just fine.
You flagged this item as spam.
Murray Steele December 7th, 2010 @ 12:06 PM
Just wondering about the rationale for saying that I have to provide the date bits for time? Why do I have to send the Year, Month and Date if all I care about is the Hour and Minute?

It just seems harsh to complain if date bits aren't given when I can use helpers to generate a form that won't have any date bits, (e.g. time_select(...., :include_date => false)). Especially if I'm coupling this to a time field where I do not care about date portions, I only have to because there's no Duration class in the stdlib to let me represent time periods without artificially saddling them to a date.

Obviously, I don't think it's a huge problem, I'd rather this patch went in than didn't. Just curious.
Aditya Sanghi December 22nd, 2010 @ 04:53 PM
@fxn, this time i'm bumping this after 1 month. Kindly have a look!
You flagged this item as spam.
fcarrettoni (at gmail) December 23rd, 2010 @ 07:34 AM
Hi all! I'm kind of new but found out that using a prompt or :include_blank => true to the time_select triggers the same multi-parameter error. I find this buggy cause if the time_select is optional in my form, it shouldn't default to 00:00 (which is a valid time). I have an events form that may or maynot have a start time, and I always get a valid start time of 00:00. Is this the correct way to use the time_select?

Is this patch going to address this too? I would gladly help to test it.
You flagged this item as spam.
fcarrettoni (at gmail) December 23rd, 2010 @ 07:38 AM
I just wanted to clearify that sometimes it triggers the multipart error and sometimes it just saves 00:00 to the db. Can't find out the logic of it yet
Aditya Sanghi December 23rd, 2010 @ 09:51 AM
@fcarrettoni, I encourage you to take this patch, apply it in your rails app and give it a go. I think we've covered almost all the cases this time. You acceptance and testing will help. If there are still issues left, we'll be keen to know and fix them. If you think the problem is solved, your upvoting will be welcome. From your last message it's not clear to me in what repeatable use case do you see this problem.
Santiago Pastorino February 12th, 2011 @ 09:58 PM
- Milestone set to “3.0.5”
[bulk edit]
Santiago Pastorino February 27th, 2011 @ 03:15 AM
- Milestone changed from “3.0.5” to “3.0.6”
[bulk edit]
David Trasbo April 5th, 2011 @ 09:36 PM
I stumbled on this bug today and it was a real pain to figure out. As I see it you can't use the :prompt option because it'll cause the 1i, 2i, and 3i (year, month, day) hidden field values to go blank, thus resulting in an invalid date error from Date.civil.

Seriously: This should be fixed in the next release. Someone even took the time to make a patch for it!

Aditya, maybe you should try submitting a pull request to the repo on GitHub. Perhaps your patch will get due attention there.
Aditya Sanghi April 6th, 2011 @ 06:55 AM
@fxn i know it's been a while. I'll rebase and reapply the patch, but can i please kindly urge you to look at this ticket again?
Xavier Noria April 6th, 2011 @ 10:30 PM
@Aditya, yes please rebase. I have a few open tickets that I plan to work on when I finish the extraction of Prototype and RJS.
Xavier Noria May 4th, 2011 @ 01:31 PM
@Aditya did you finally rebase? The diff does not apply clean right now.