Unquoted query generated by through-association scope #1361

Closed
wants to merge 1,218 commits into
from

Conversation

Projects
None yet
@sirlantis

Through-association owner's primary key wasn't quoted. This generates invalid SQL if the record wasn't saved yet (i.e. the primary key's value is nil) and you try to access the relation (should return an empty result).

Real-world example of generated sql:

SELECT `tags`.*
  FROM `tags`
  INNER JOIN `taggings` ON `tags`.id = `taggings`.tag_id
  WHERE
    ((`taggings`.taggable_id = ) AND (`taggings`.taggable_type = 'Ticket'))
    AND (taggings.context = 'tags' AND taggings.tagger_id IS NULL)

jamis and others added some commits Jan 19, 2011

scrub instance variables from test cases on teardown
this prevents test state from accumulating, resulting in leaked
objects and slow tests due to overactive GC.
rein in GC during tests by making them run (at most) once per second
this can provide a significant performance boost during testing, by
preventing the GC from running too frequently.
Added a testcase for bug [#5329]
Signed-off-by: José Valim <jose.valim@gmail.com>
Merge remote branch 'jonleighton/deprecate_habtm_attributes-3-0-stabl…
…e' into 3-0-stable

* jonleighton/deprecate_habtm_attributes-3-0-stable:
  Added deprecation warning for has_and_belongs_to_many associations where the join table has additional attributes other than the keys. Access to these attributes is removed in 3.1. Please use has_many :through instead.
Be sure to javascript_escape the email address to prevent apostrophes…
… inadvertently causing javascript errors.

This fixes CVE-2011-0446
limit() should sanitize limit values
This fixes CVE-2011-0448
Change the CSRF whitelisting to only apply to get requests
Unfortunately the previous method of browser detection and XHR whitelisting is unable to prevent requests issued from some Flash animations and Java applets.  To ease the work required to include the CSRF token in ajax requests rails now supports providing the token in a custom http header:

 X-CSRF-Token: ...

This fixes CVE-2011-0447
Make before_type_cast available for datetime fields
Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
before_type_cast on Datetime tests for Mysql2Adapter
Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
Additionally trigger i18n configuration setup before any eager loading [
#6353 state:resolved]

This handles the case where config.cache_classes is true and classes
are loaded before the I18n load path has had a chance to be populated.

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
Ensure I18n setup is only executed once if triggered on eager loading [
…#6353 state:resolved]

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
Add a test for 'render :layout'
To make sure it will show block contents if it is placed after 'render
:partial'

[#5557 state:resolved]

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
The type_cast_calculated_value method will trust DB types before cast…
…ing to a BigDecimal.

[#6365 state:committed]

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
Fixing ordering of HABTM association deletion [#6191 state:resolved]
Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
@nragaz

This comment has been minimized.

Show comment
Hide comment
@nragaz

nragaz Feb 6, 2011

Contributor

original_time.dup gets called even if original_time is nil, resulting in an exception (can't dup NilClass). An unless original_time.nil? would help.

original_time.dup gets called even if original_time is nil, resulting in an exception (can't dup NilClass). An unless original_time.nil? would help.

This comment has been minimized.

Show comment
Hide comment
@amatsuda

amatsuda Feb 6, 2011

Member

Oh, thanks @nragaz, that was my mistake...

Member

amatsuda replied Feb 6, 2011

Oh, thanks @nragaz, that was my mistake...

This comment has been minimized.

Show comment
Hide comment
@amatsuda

amatsuda Feb 6, 2011

Member
From 9138bb1ad26d8b0c8a12722f9ac07e5c433f3f9f Mon Sep 17 00:00:00 2001
From: Akira Matsuda <ronnie@dio.jp>
Date: Mon, 7 Feb 2011 08:29:06 +0900
Subject: [PATCH] avoid nil.dup

---
 .../attribute_methods/time_zone_conversion.rb      |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
index a72eecb..76218d2 100644
--- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
+++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
@@ -41,7 +41,7 @@ module ActiveRecord
             if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name])
               method_body, line = <<-EOV, __LINE__ + 1
                 def #{attr_name}=(original_time)
-                  time = original_time.dup
+                  time = original_time.dup unless original_time.nil?
                   unless time.acts_like?(:time)
                     time = time.is_a?(String) ? Time.zone.parse(time) : time.to_time rescue time
                   end
-- 
1.7.3.5
Member

amatsuda replied Feb 6, 2011

From 9138bb1ad26d8b0c8a12722f9ac07e5c433f3f9f Mon Sep 17 00:00:00 2001
From: Akira Matsuda <ronnie@dio.jp>
Date: Mon, 7 Feb 2011 08:29:06 +0900
Subject: [PATCH] avoid nil.dup

---
 .../attribute_methods/time_zone_conversion.rb      |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
index a72eecb..76218d2 100644
--- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
+++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
@@ -41,7 +41,7 @@ module ActiveRecord
             if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name])
               method_body, line = <<-EOV, __LINE__ + 1
                 def #{attr_name}=(original_time)
-                  time = original_time.dup
+                  time = original_time.dup unless original_time.nil?
                   unless time.acts_like?(:time)
                     time = time.is_a?(String) ? Time.zone.parse(time) : time.to_time rescue time
                   end
-- 
1.7.3.5

This comment has been minimized.

Show comment
Hide comment
@nragaz

nragaz Feb 6, 2011

Contributor

Thank you! Was just about to do a patch myself.

Contributor

nragaz replied Feb 6, 2011

Thank you! Was just about to do a patch myself.

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Feb 7, 2011

Member

Can you provide a patch with a test?, thanks.

Member

spastorino replied Feb 7, 2011

Can you provide a patch with a test?, thanks.

This comment has been minimized.

Show comment
Hide comment
@stid

stid Feb 9, 2011

Guys, this was released (not patched) in 3.0.4, I'm getting this error in authologic when he try to update the last_login column after a new session/user is created and in a bunch of tests.

Update: Sorry, my mistake, I was using the 30-stable branch. This was not released in 3.0.4 final.

stid replied Feb 9, 2011

Guys, this was released (not patched) in 3.0.4, I'm getting this error in authologic when he try to update the last_login column after a new session/user is created and in a bunch of tests.

Update: Sorry, my mistake, I was using the 30-stable branch. This was not released in 3.0.4 final.

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Feb 28, 2011

Contributor

This breaks when using the home_run gem since Date#dup (Date.allocate) is not implemented: "TypeError: allocator undefined for Date"

What exactly is being achieved by duping the Date? Is there another way this can be achieved?

Contributor

alindeman replied Feb 28, 2011

This breaks when using the home_run gem since Date#dup (Date.allocate) is not implemented: "TypeError: allocator undefined for Date"

What exactly is being achieved by duping the Date? Is there another way this can be achieved?

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Feb 28, 2011

Contributor

Actually, looks like it's being reported as a bug on home_run: https://github.com/jeremyevans/home_run/issues#issue/21

Contributor

alindeman replied Feb 28, 2011

Actually, looks like it's being reported as a bug on home_run: https://github.com/jeremyevans/home_run/issues#issue/21

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap Mar 1, 2011

Contributor

Although that is an issue for home_run to fix, as dup is valid on a Date instance. I have submitted a ticket which removes the dup because it is not necessary. No in-place change is made to the time value which could effect the original_time value.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6489-fix-before_type_cast-for-timezone-aware-datetime-attributes#ticket-6489-3

Contributor

adzap replied Mar 1, 2011

Although that is an issue for home_run to fix, as dup is valid on a Date instance. I have submitted a ticket which removes the dup because it is not necessary. No in-place change is made to the time value which could effect the original_time value.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6489-fix-before_type_cast-for-timezone-aware-datetime-attributes#ticket-6489-3

@adzap

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap Feb 7, 2011

Contributor

You do a to_s on the before_type_cast value which will convert the Time instance back to a string. So this does not test that a string value is stored for the before_type_cast when the original_time is a valid time string.

You do a to_s on the before_type_cast value which will convert the Time instance back to a string. So this does not test that a string value is stored for the before_type_cast when the original_time is a valid time string.

This comment has been minimized.

Show comment
Hide comment
@amatsuda

amatsuda Feb 7, 2011

Member

You're absolutely right.
Just pushed the fix. 65e08cf
Thanks for your advice!

Member

amatsuda replied Feb 7, 2011

You're absolutely right.
Just pushed the fix. 65e08cf
Thanks for your advice!

This comment has been minimized.

Show comment
Hide comment
@adzap

adzap Feb 7, 2011

Contributor

No problem. But I am surprised that this change actually passes. From the code, if the time string is successfully parsed, the time variable will be Time instance. Therefore when (time || original_time) is evaluated, it will pass the time instance to be stored by write_attribute. So when you call before_type_cast you should get the same Time instance, and not the original string value.

Am I missing something?

Contributor

adzap replied Feb 7, 2011

No problem. But I am surprised that this change actually passes. From the code, if the time string is successfully parsed, the time variable will be Time instance. Therefore when (time || original_time) is evaluated, it will pass the time instance to be stored by write_attribute. So when you call before_type_cast you should get the same Time instance, and not the original string value.

Am I missing something?

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Feb 8, 2011

Contributor

There should be a case here for an Arel::Sql::Literal to pass thru untouched?

There should be a case here for an Arel::Sql::Literal to pass thru untouched?

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Feb 9, 2011

Contributor

I'll be committing a patch that adds :SQLServerAdapter too.

Contributor

metaskills commented on ff00cd2 Feb 9, 2011

I'll be committing a patch that adds :SQLServerAdapter too.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt May 25, 2011

Contributor

Changelogs ftw

Contributor

parndt commented on eecbc10 May 25, 2011

Changelogs ftw

tenderlove and others added some commits May 25, 2011

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik May 26, 2011

Contributor

👍 Nice one! Congratulations on your first commit in Rails.

Contributor

sferik commented on 83f257f May 26, 2011

👍 Nice one! Congratulations on your first commit in Rails.

@pacoguzman

This comment has been minimized.

Show comment
Hide comment
@pacoguzman

pacoguzman May 26, 2011

Contributor

IMHO is possible to use 0.2.7, ins't it?

IMHO is possible to use 0.2.7, ins't it?

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove May 26, 2011

Member

Yes. The ~> constraint lets you use "0.2.x" that is greater than or equal to "0.2.6".

Member

tenderlove replied May 26, 2011

Yes. The ~> constraint lets you use "0.2.x" that is greater than or equal to "0.2.6".

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 27, 2011

Contributor

It seems something is wrong with your pull request. :P

Contributor

josevalim commented May 27, 2011

It seems something is wrong with your pull request. :P

@josevalim josevalim closed this May 27, 2011

@sirlantis

This comment has been minimized.

Show comment
Hide comment
@sirlantis

sirlantis May 27, 2011

Whoops. Github exploded. Fixed the Pull-Request (now 1362). Sorry for the mess.

Whoops. Github exploded. Fixed the Pull-Request (now 1362). Sorry for the mess.

@mtodd

This comment has been minimized.

Show comment
Hide comment
@mtodd

mtodd Jun 19, 2011

Contributor

Was this line left in here accidentally?

Was this line left in here accidentally?

This comment has been minimized.

Show comment
Hide comment
@mtodd

mtodd Jun 19, 2011

Contributor

Heh, nevermind, misread line 544 and realized once I posted this comment. ❤️

Contributor

mtodd replied Jun 19, 2011

Heh, nevermind, misread line 544 and realized once I posted this comment. ❤️

jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011

@lawrencepit

This comment has been minimized.

Show comment
Hide comment
@lawrencepit

lawrencepit Jul 15, 2011

Contributor

The problem with this implementation (compared to rails2) is that read_attribute is now in the top 5 responsible for creating the most objects (in a typical rails app), and thereby being a burden on the GC; i.e. it slows things down.

Contributor

lawrencepit commented on a3639be Jul 15, 2011

The problem with this implementation (compared to rails2) is that read_attribute is now in the top 5 responsible for creating the most objects (in a typical rails app), and thereby being a burden on the GC; i.e. it slows things down.

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