#4942 ActiveSupport::TimeZone.[] incorrectly stores bogus values in zones_map - 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.

#4942 ✓resolved

ActiveSupport::TimeZone.[] incorrectly stores bogus values in zones_map

Reported by Leigh Caplan | June 23rd, 2010 @ 03:39 AM | in 2.3.9

>> ActiveSupport::TimeZone['bogus']
=> nil
>> ActiveSupport::TimeZone.zones_map.key?('bogus')
=> true

This really bites you if the value ends up being stored, but ActiveSupport::TimeZone.all hasn't been called yet. In this case, ActiveSupport::TimeZone.zones_map.values gets sorted, and blows up because nil has no <=> defined.

This was never an issue before 49943a712080644969d1, because ZONES and US_ZONES were pre-loaded before any shenanigans of this sort could take place.

I've included a patch that should apply to both 2-3-stable and master. Note that this failure mode doesn't rear its head in master, as long as you're using a newer version of the tzinfo gem- trying to look up a nonexistent time zone will raise an error. That being said, if the tzinfo behavior changed tomrrow, Rails would again be vulnerable due to the way this code is written.

0001-prevent-bogus-time-zones-from-storing-nil-keys-in-ac.patch 1.8 KB

Comments and changes to this ticket

Rohit Arondekar June 23rd, 2010 @ 03:45 AM
- State changed from “new” to “open”
- Assigned user set to “Prem Sichanugrist (sikachu)”
Prem Sichanugrist (sikachu) June 23rd, 2010 @ 04:40 PM
- State changed from “open” to “incomplete”
- Assigned user changed from “Prem Sichanugrist (sikachu)” to “Santiago Pastorino”
This patch does apply on master, but the test fails.

I can see the code in master (in #lookup) that it will create the time zone if it's not exists in the mapping. I don't know that this is the intended behavior or not.

I'm assigning this to Santiago Pastorino to have a look on this one.

By the way, in your opinion do you think it would be better for Rails to update the tzinfo gem to require at least 0.3.22 then?
You flagged this item as spam.
Leigh Caplan June 23rd, 2010 @ 06:36 PM
I've included a test that passes in master... I just stubbed out the lookup method to return nil, to ensure that we're not explicitly relying on the TZInfo to do the right thing. And I don't think that storing nil values in @zones_map is the intended behavior. If it is, then it was probably done without understanding how it can break other time zone functionality.

For a little background, our app had some calendar events with time zones which had been accidentally sanitized, so the ampersands were HTML encoded. The current TZInfo behavior is correct, and if it was included in Rails 2.3.8, an exception would've been raised as soon as we tried to use the bad time zone. Unfortunately, what actually happened was that some of our backends fielded these calendar event requests as soon as they started up, and nil values "poisoned" the @zones_map. The next time someone tried to draw a time zone menu, we got an exception while trying to sort @zones_map. As you can imagine, this was a really difficult issue to track down, and it required that we roll back to 2.3.5 until the problem was sorted out.

FWIW, I think it's a great idea to update the tzinfo dependency to 0.3.22, but only because it gives us the latest tzdata. I'm pretty sure this behavior is the same in 0.3.16.
- time_zone_map_fix.patch 3.1 KB
Repository June 28th, 2010 @ 06:40 PM
- Importance changed from “” to “Low”
(from [97a92a4cfddf819357ca09b4a91a5937b044e4d8]) test that unknown zones don't store mapping keys

[#4942]

Signed-off-by: Santiago Pastorino santiago@wyeworks.com
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/97a92a4cfddf819357ca09b4a91a59...
Repository June 28th, 2010 @ 06:40 PM
- State changed from “incomplete” to “committed”
(from [b2633f9f9323f5d5eca1c2c94bb7ae88ad276cda]) Don't store incorrect values in zones_map

[#4942 state:committed]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/b2633f9f9323f5d5eca1c2c94bb7ae...
Santiago Pastorino June 28th, 2010 @ 06:48 PM
- Milestone set to “2.3.9”
- State changed from “committed” to “open”
I will push a backport in a couple of minutes
Santiago Pastorino June 28th, 2010 @ 07:33 PM
- Assigned user changed from “Santiago Pastorino” to “José Valim”
- backport-2-3-stable.patch 2 KB
Repository June 28th, 2010 @ 09:26 PM
- State changed from “open” to “committed”
(from [70af7efa16fa429f7bbc3acfc1c587c8655edf7e]) Don't store incorrect values in zones_map backport

[#4942 state:committed]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/70af7efa16fa429f7bbc3acfc1c587...
Repository June 28th, 2010 @ 09:26 PM
(from [80473e035a166d270a204aedaf01c2b80415b8e4]) test that unknown zones don't store mapping keys

[#4942]

Signed-off-by: Santiago Pastorino santiago@wyeworks.com
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/80473e035a166d270a204aedaf01c2...
You flagged this item as spam.
Leigh Caplan June 28th, 2010 @ 11:52 PM
Hey guys,

The 2.3 backport patch causes some additional unexpected behavior. It will never use the stored value in zones_map, and create a new TimeZone object each time #[] is invoked. Here's a patch to fix it.
- lookup-fix-2-3-stable.patch 1 KB
José Valim June 28th, 2010 @ 11:59 PM
Could you please include tests in the patch? Thanks!
Santiago Pastorino June 29th, 2010 @ 01:28 AM
Leigh your patch is ok but it's a performance improvement.
```
-            if tz = lookup(arg)
-              zones_map[arg] ||= tz
```
From this code you're removing note the ||=, if zones_map[arg] have something it will be returned before the assignation to tz. But i agree we can check zones_map[arg] before to not do some unneded lookups.

So José that doesn't need a test, all the things we need to test are already tested.
You flagged this item as spam.
Leigh Caplan June 29th, 2010 @ 01:53 AM
Here's a patch with both the lookup fix and a test. I couldn't think of any way other than mocking out the call to find_tzinfo seeing as object apparently doesn't get stored anywhere. If someone else has any better ideas, I'd be glad to hear them.
- lookup-tests-fix-2-3-stable.patch 2.3 KB
You flagged this item as spam.
Leigh Caplan June 29th, 2010 @ 01:54 AM
- Assigned user changed from “José Valim” to “Santiago Pastorino”
Santiago Pastorino June 29th, 2010 @ 02:21 AM
- Assigned user changed from “Santiago Pastorino” to “José Valim”
- State changed from “committed” to “verified”
We don't need this test for my it's ok to apply this patch

https://rails.lighthouseapp.com/projects/8994/tickets/4942/a/579448...
Repository June 29th, 2010 @ 04:57 PM
- State changed from “verified” to “resolved”
(from [78e4d88c7071c633ee1eb68f49b90719aa198eaa]) Rewrite the clause to pluck the existing value from zones_map before performing a lookup. [#4942 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/78e4d88c7071c633ee1eb68f49b907...
You flagged this item as spam.
plind July 1st, 2010 @ 01:02 PM
Does this solve this problem: http://stackoverflow.com/questions/3027704/nomethoderror-for-time-z... ?
You flagged this item as spam.
Leigh Caplan July 23rd, 2010 @ 09:35 PM
@plind yes, sorry I didn't see this for a month. This resolves the issue.