kotovalexarian-likes-github/rails--rails
1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00
Code Releases Activity

Allow column name with function (e.g. length(title)) as safe SQL string

Browse source 
Currently, almost all "Dangerous query method" warnings are false alarm.
As long as almost all the warnings are false alarm, developers think
"Let's ignore the warnings by using `Arel.sql()`, it actually is false
alarm in practice.", so I think we should effort to reduce false alarm
in order to make the warnings valuable.

This allows column name with function (e.g. `length(title)`) as safe SQL
string, which is very common false alarm pattern, even in the our
codebase.

Related 6c82b6c99, 6607ecb2a, #36420.

Fixes #32995.
This commit is contained in:
Ryuta Kamizono 2019-06-09 08:01:10 +09:00
parent 6607ecb2a1
commit 64d8c54e16
7 changed files with 67 additions and 42 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
View file

@ -158,7 +158,10 @@ module ActiveRecord
COLUMN_NAME = /
\A
(
(?:\w+\.)?\w+
(?:
# table_name.column_name | function(one or no argument)
((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\)
)
(?:(?:\s+AS)?\s+\w+)?
)
(?:\s*,\s*\g<1>)*
@ -179,7 +182,10 @@ module ActiveRecord
COLUMN_NAME_WITH_ORDER = /
\A
(
(?:\w+\.)?\w+
(?:
# table_name.column_name | function(one or no argument)
((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\)
)
(?:\s+ASC|\s+DESC)?
(?:\s+NULLS\s+(?:FIRST|LAST))?
)

10
activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
View file

@ -43,7 +43,10 @@ module ActiveRecord
COLUMN_NAME = /
\A
(
(?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)
(?:
# `table_name`.`column_name` | function(one or no argument)
((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)) | \w+\((?:|\g<2>)\)
)
(?:(?:\s+AS)?\s+(?:\w+|`\w+`))?
)
(?:\s*,\s*\g<1>)*
@ -53,7 +56,10 @@ module ActiveRecord
COLUMN_NAME_WITH_ORDER = /
\A
(
(?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)
(?:
# `table_name`.`column_name` | function(one or no argument)
((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)) | \w+\((?:|\g<2>)\)
)
(?:\s+ASC|\s+DESC)?
)
(?:\s*,\s*\g<1>)*

10
activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
View file

@ -89,7 +89,10 @@ module ActiveRecord
COLUMN_NAME = /
\A
(
(?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?
(?:
# "table_name"."column_name"::type_name | function(one or no argument)::type_name
((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?) | \w+\((?:|\g<2>)\)(?:::\w+)?
)
(?:(?:\s+AS)?\s+(?:\w+|"\w+"))?
)
(?:\s*,\s*\g<1>)*
@ -99,7 +102,10 @@ module ActiveRecord
COLUMN_NAME_WITH_ORDER = /
\A
(
(?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?
(?:
# "table_name"."column_name"::type_name | function(one or no argument)::type_name
((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?) | \w+\((?:|\g<2>)\)(?:::\w+)?
)
(?:\s+ASC|\s+DESC)?
(?:\s+NULLS\s+(?:FIRST|LAST))?
)

10
activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
View file

@ -56,7 +56,10 @@ module ActiveRecord
COLUMN_NAME = /
\A
(
(?:\w+\.|"\w+"\.)?(?:\w+|"\w+")
(?:
# "table_name"."column_name" | function(one or no argument)
((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")) | \w+\((?:|\g<2>)\)
)
(?:(?:\s+AS)?\s+(?:\w+|"\w+"))?
)
(?:\s*,\s*\g<1>)*
@ -66,7 +69,10 @@ module ActiveRecord
COLUMN_NAME_WITH_ORDER = /
\A
(
(?:\w+\.|"\w+"\.)?(?:\w+|"\w+")
(?:
# "table_name"."column_name" | function(one or no argument)
((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")) | \w+\((?:|\g<2>)\)
)
(?:\s+ASC|\s+DESC)?
)
(?:\s*,\s*\g<1>)*

8
activerecord/test/cases/associations/eager_test.rb
View file

@ -968,14 +968,14 @@ class EagerAssociationTest < ActiveRecord::TestCase
posts(:thinking, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: Arel.sql("UPPER(posts.title)"), limit: 2, offset: 1
order: "UPPER(posts.title)", limit: 2, offset: 1
).to_a
)
assert_equal(
posts(:sti_post_and_comments, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: Arel.sql("UPPER(posts.title) DESC"), limit: 2, offset: 1
order: "UPPER(posts.title) DESC", limit: 2, offset: 1
).to_a
)
end
@ -985,14 +985,14 @@ class EagerAssociationTest < ActiveRecord::TestCase
posts(:thinking, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: [Arel.sql("UPPER(posts.title)"), "posts.id"], limit: 2, offset: 1
order: ["UPPER(posts.title)", "posts.id"], limit: 2, offset: 1
).to_a
)
assert_equal(
posts(:sti_post_and_comments, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: [Arel.sql("UPPER(posts.title) DESC"), "posts.id"], limit: 2, offset: 1
order: ["UPPER(posts.title) DESC", "posts.id"], limit: 2, offset: 1
).to_a
)
end

6
activerecord/test/cases/relations_test.rb
View file

@ -298,7 +298,7 @@ class RelationTest < ActiveRecord::TestCase
end
def test_reverse_order_with_function
topics = Topic.order(Arel.sql("length(title)")).reverse_order
topics = Topic.order("length(title)").reverse_order
assert_equal topics(:second).title, topics.first.title
end
@ -1696,7 +1696,7 @@ class RelationTest < ActiveRecord::TestCase
scope = Post.order("comments.body asc")
assert_equal ["comments"], scope.references_values
scope = Post.order(Arel.sql("foo(comments.body)"))
scope = Post.order("foo(comments.body)")
assert_equal [], scope.references_values
end
@ -1721,7 +1721,7 @@ class RelationTest < ActiveRecord::TestCase
scope = Post.reorder("comments.body asc")
assert_equal %w(comments), scope.references_values
scope = Post.reorder(Arel.sql("foo(comments.body)"))
scope = Post.reorder("foo(comments.body)")
assert_equal [], scope.references_values
end

55
activerecord/test/cases/unsafe_raw_sql_test.rb
View file

@ -141,7 +141,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "order: disallows invalid column name" do
with_unsafe_raw_sql_disabled do
assert_raises(ActiveRecord::UnknownAttributeReference) do
Post.order("len(title) asc").pluck(:id)
Post.order("REPLACE(title, 'misc', 'zzzz') asc").pluck(:id)
end
end
end
@ -157,7 +157,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "order: disallows invalid column with direction" do
with_unsafe_raw_sql_disabled do
assert_raises(ActiveRecord::UnknownAttributeReference) do
Post.order("len(title)" => :asc).pluck(:id)
Post.order("REPLACE(title, 'misc', 'zzzz')" => :asc).pluck(:id)
end
end
end
@ -190,7 +190,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "order: disallows invalid Array arguments" do
with_unsafe_raw_sql_disabled do
assert_raises(ActiveRecord::UnknownAttributeReference) do
Post.order(["author_id", "length(title)"]).pluck(:id)
Post.order(["author_id", "REPLACE(title, 'misc', 'zzzz')"]).pluck(:id)
end
end
end
@ -198,8 +198,8 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "order: allows valid Array arguments" do
ids_expected = Post.order(Arel.sql("author_id, length(title)")).pluck(:id)
ids_depr = with_unsafe_raw_sql_deprecated { Post.order(["author_id", Arel.sql("length(title)")]).pluck(:id) }
ids_disabled = with_unsafe_raw_sql_disabled { Post.order(["author_id", Arel.sql("length(title)")]).pluck(:id) }
ids_depr = with_unsafe_raw_sql_deprecated { Post.order(["author_id", "length(title)"]).pluck(:id) }
ids_disabled = with_unsafe_raw_sql_disabled { Post.order(["author_id", "length(title)"]).pluck(:id) }
assert_equal ids_expected, ids_depr
assert_equal ids_expected, ids_disabled
@ -208,7 +208,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "order: logs deprecation warning for unrecognized column" do
with_unsafe_raw_sql_deprecated do
assert_deprecated(/Dangerous query method/) do
Post.order("length(title)")
Post.order("REPLACE(title, 'misc', 'zzzz')")
end
end
end
@ -223,11 +223,11 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
assert_equal titles_expected, titles_disabled
end
test "pluck: allows string column name with alias" do
titles_expected = Post.pluck(Arel.sql("title"))
test "pluck: allows string column name with function and alias" do
titles_expected = Post.pluck(Arel.sql("UPPER(title)"))
titles_depr = with_unsafe_raw_sql_deprecated { Post.pluck("title AS posts_title") }
titles_disabled = with_unsafe_raw_sql_disabled { Post.pluck("title AS posts_title") }
titles_depr = with_unsafe_raw_sql_deprecated { Post.pluck("UPPER(title) AS title") }
titles_disabled = with_unsafe_raw_sql_disabled { Post.pluck("UPPER(title) AS title") }
assert_equal titles_expected, titles_depr
assert_equal titles_expected, titles_disabled
@ -297,7 +297,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "pluck: disallows invalid column name" do
with_unsafe_raw_sql_disabled do
assert_raises(ActiveRecord::UnknownAttributeReference) do
Post.pluck("length(title)")
Post.pluck("REPLACE(title, 'misc', 'zzzz')")
end
end
end
@ -305,7 +305,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "pluck: disallows invalid column name amongst valid names" do
with_unsafe_raw_sql_disabled do
assert_raises(ActiveRecord::UnknownAttributeReference) do
Post.pluck(:title, "length(title)")
Post.pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
end
end
end
@ -313,7 +313,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "pluck: disallows invalid column names with includes" do
with_unsafe_raw_sql_disabled do
assert_raises(ActiveRecord::UnknownAttributeReference) do
Post.includes(:comments).pluck(:title, "length(title)")
Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
end
end
end
@ -328,24 +328,25 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
test "pluck: logs deprecation warning" do
with_unsafe_raw_sql_deprecated do
assert_deprecated(/Dangerous query method/) do
Post.includes(:comments).pluck(:title, "length(title)")
Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
end
end
end
def with_unsafe_raw_sql_disabled(&blk)
with_config(:disabled, &blk)
end
private
def with_unsafe_raw_sql_disabled(&block)
with_config(:disabled, &block)
end
def with_unsafe_raw_sql_deprecated(&blk)
with_config(:deprecated, &blk)
end
def with_unsafe_raw_sql_deprecated(&block)
with_config(:deprecated, &block)
end
def with_config(new_value, &blk)
old_value = ActiveRecord::Base.allow_unsafe_raw_sql
ActiveRecord::Base.allow_unsafe_raw_sql = new_value
blk.call
ensure
ActiveRecord::Base.allow_unsafe_raw_sql = old_value
end
def with_config(new_value, &block)
old_value = ActiveRecord::Base.allow_unsafe_raw_sql
ActiveRecord::Base.allow_unsafe_raw_sql = new_value
yield
ensure
ActiveRecord::Base.allow_unsafe_raw_sql = old_value
end
end