time_select not interpreting the multi-parameter correctly #667

Closed
lighthouse-import opened this Issue May 16, 2011 · 56 comments

3 participants

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/4346
Created by khagimoto - 2010-12-28 03:36:18 UTC

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/thread/dfc64e24151c7880

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.

@lighthouse-import

Imported from Lighthouse.
Comment by ultimasnake - 2010-10-04 17:38:53 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by David Trasbo - 2010-10-08 08:26:37 UTC

Please provide a patch with a fix/failing test.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-08 14:39:14 UTC

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 ...

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-08 19:51:52 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2010-10-08 21:22:01 UTC

Aditya can you provide a patch please?.
And this is for 3.0 I guess right?

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-09 07:53:55 UTC

I think this is a problem in 2.3.X also and yes I will provide a patch for that too.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-09 08:02:39 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Rohit Arondekar - 2010-10-09 08:23:46 UTC

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.html#method-i-time_select

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-09 08:42:49 UTC

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/503-add-deny_hidden-option-to-date-helpers-so-rails-doesn-t-render-them

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Rohit Arondekar - 2010-10-09 09:01:07 UTC

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! :)

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-09 15:57:38 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Xavier Noria - 2010-10-10 09:12:26 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-10 11:16:39 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Xavier Noria - 2010-10-10 11:35:46 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-10 12:26:39 UTC

Xavier, why would one not use datetime_select instead for this? I've updated the documentation with more conviction this time :)

@lighthouse-import

Imported from Lighthouse.
Comment by Jeff Kreeftmeijer - 2010-10-10 19:30:19 UTC

Please use the "patch" tag when adding a patch to make sure patched tickets end up in the open patches bin. :)

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-13 06:58:28 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-13 07:08:45 UTC

(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 ;)

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-13 14:31:15 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-13 15:32:44 UTC

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).

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-13 19:39:59 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-13 21:47:25 UTC

Good point. I'll give that I try.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-14 04:59:17 UTC

Andrea, My patch takes care of filling in the missing bits with nil.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-15 07:13:25 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-15 17:01:27 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-16 19:13:37 UTC

+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.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-16 19:17:05 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-16 19:41:04 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-16 19:46:44 UTC

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 ).

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-16 19:56:18 UTC

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).

@lighthouse-import

Imported from Lighthouse.
Comment by Andrea Campi - 2010-10-16 20:24:38 UTC

+1 from me :)

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-20 08:27:55 UTC

Changing to 3.0.2 as it's a bugfix.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-23 14:42:32 UTC

@fxn, what do you think? would appreciate your views when you get a chance.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-10-29 06:58:33 UTC

Shameless bump for attention. :) I think this is important enough for someone from core to comment.

@lighthouse-import

Imported from Lighthouse.
Comment by Xavier Noria - 2010-10-29 07:34:49 UTC

Hey thanks for the ping.

I've not replied yet, but it is in my plate no worries.

@lighthouse-import

Imported from Lighthouse.
Comment by Jeff Kreeftmeijer - 2010-11-01 17:01:18 UTC

Automatic cleanup of spam.

@lighthouse-import

Imported from Lighthouse.
Comment by Jeff Kreeftmeijer - 2010-11-07 16:51:51 UTC

Automatic cleanup of spam.

@lighthouse-import

Imported from Lighthouse.
Comment by Jeff Kreeftmeijer - 2010-11-08 08:28:08 UTC

Automatic cleanup of spam.

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2010-11-15 21:04:51 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-11-17 11:59:54 UTC

Rebased against master (but secretly and gently bumping).

Forewarning: will bump again in 2 weeks if nothing happens in the meantime. :)

@lighthouse-import

Imported from Lighthouse.
Comment by lakshmanan - 2010-11-25 19:08:01 UTC

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 ?

@lighthouse-import

Imported from Lighthouse.
Comment by lakshmanan - 2010-11-25 19:41:59 UTC

Here is how I fixed it in my form for now. Hope this helps.

https://gist.github.com/715819

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-11-26 09:21:15 UTC

@lakshmanan your usage of :ignore_date => false seems just fine.

@lighthouse-import

Imported from Lighthouse.
Comment by Murray Steele - 2010-12-07 12:06:23 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-12-22 16:53:23 UTC

@fxn, this time i'm bumping this after 1 month. Kindly have a look!

@lighthouse-import

Imported from Lighthouse.
Comment by fcarrettoni (at gmail) - 2010-12-23 07:34:46 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by fcarrettoni (at gmail) - 2010-12-23 07:38:02 UTC

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

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2010-12-23 09:51:41 UTC

@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.

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-12 21:58:21 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-27 03:15:38 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by David Trasbo - 2011-04-05 20:36:29 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Aditya Sanghi - 2011-04-06 05:55:44 UTC

@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?

@lighthouse-import

Imported from Lighthouse.
Comment by Xavier Noria - 2011-04-06 21:30:05 UTC

@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.

@lighthouse-import

Imported from Lighthouse.
Comment by Xavier Noria - 2011-05-04 12:31:04 UTC

@Aditya did you finally rebase? The diff does not apply clean right now.

@asanghi

This ticket can be closed. It's already in Rails Master now. #396

@josevalim josevalim closed this May 16, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment