New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow raw SQL in dangerous AR methods #27947

Closed
wants to merge 42 commits into
base: master
from
Commits
Jump to file or symbol
Failed to load files and symbols.
+437 −33
Diff settings

Always

Just for now

@@ -167,6 +167,46 @@ def attribute_names
end
end
# Regexp whitelist. Matches the following:
# "#{table_name}.#{column_name}"
# "#{column_name}"
COLUMN_NAME_WHITELIST = /\A(?:\w+\.)?\w+\z/i
# Regexp whitelist. Matches the following:
# "#{table_name}.#{column_name}"
# "#{table_name}.#{column_name} #{direction}"
# "#{column_name}"
# "#{column_name} #{direction}"
COLUMN_NAME_ORDER_WHITELIST = /\A(?:\w+\.)?\w+(?:\s+asc|\s+desc)?\z/i
def enforce_raw_sql_whitelist(args, whitelist: COLUMN_NAME_WHITELIST) # :nodoc:
unexpected = args.reject do |arg|
arg.kind_of?(Arel::Node) ||
arg.is_a?(Arel::Nodes::SqlLiteral) ||
arg.is_a?(Arel::Attributes::Attribute) ||
arg.to_s.split(/\s*,\s*/).all? { |part| whitelist.match?(part) }
end
return if unexpected.none?
if allow_unsafe_raw_sql == :deprecated
ActiveSupport::Deprecation.warn(
"Dangerous query method (method whose arguments are used as raw " \
"SQL) called with non-attribute argument(s): " \
"#{unexpected.map(&:inspect).join(", ")}. Non-attribute " \
"arguments will be disallowed in Rails 6.0. This method should " \
"not be called with user-provided values, such as request " \
"parameters or model attributes. Known-safe values can be passed " \
"by wrapping them in Arel.sql()."
)
else
raise(ActiveRecord::UnknownAttributeReference,
"Query method called with non-attribute argument(s): " +
unexpected.map(&:inspect).join(", ")
)
end
end
# Returns true if the given attribute exists, otherwise false.
#
# class Person < ActiveRecord::Base
@@ -76,6 +76,15 @@ def self.configurations
# scope being ignored is error-worthy, rather than a warning.
mattr_accessor :error_on_ignored_order, instance_writer: false, default: false
##
# :singleton-method:
# Specify the behavior for unsafe raw query methods. Values are as follows
# deprecated - Warnings are logged when unsafe raw SQL is passed to
# query methods.
# disabled - Unsafe raw SQL passed to query methods results in
# UnknownAttributeReference exception.
mattr_accessor :allow_unsafe_raw_sql, instance_writer: false, default: :deprecated
##
# :singleton-method:
# Specify whether or not to use timestamps for migration versions
@@ -335,6 +335,31 @@ class Deadlocked < TransactionRollbackError
class IrreversibleOrderError < ActiveRecordError
end
# UnknownAttributeReference is raised when an unknown and potentially unsafe
# value is passed to a query method when allow_unsafe_raw_sql is set to
# :disabled. For example, passing a non column name value to a relation's
# #order method might cause this exception.
#
# When working around this exception, caution should be taken to avoid SQL
# injection vulnerabilities when passing user-provided values to query
# methods. Known-safe values can be passed to query methods by wrapping them
# in Arel.sql.
#
# For example, with allow_unsafe_raw_sql set to :disabled, the following
# code would raise this exception:
#
# Post.order("length(title)").first
#
# The desired result can be accomplished by wrapping the known-safe string
# in Arel.sql:
#
# Post.order(Arel.sql("length(title)")).first
#
# Again, such a workaround should *not* be used when passing user-provided
# values, such as request parameters or model attributes to query methods.
class UnknownAttributeReference < ActiveRecordError
end
# TransactionTimeout will be raised when lock wait timeout expires.
# Wait time value is set by innodb_lock_wait_timeout.
class TransactionTimeout < StatementInvalid
@@ -183,6 +183,7 @@ def pluck(*column_names)
relation = apply_join_dependency(construct_join_dependency)
relation.pluck(*column_names)
else
enforce_raw_sql_whitelist(column_names)
relation = spawn
relation.select_values = column_names.map { |cn|
@klass.has_attribute?(cn) || @klass.attribute_alias?(cn) ? arel_attribute(cn) : cn
@@ -295,6 +295,7 @@ def order(*args)
spawn.order!(*args)
end
# Same as #order but operates on relation in-place instead of copying.
def order!(*args) # :nodoc:
preprocess_order_args(args)
@@ -316,6 +317,7 @@ def reorder(*args)
spawn.reorder!(*args)
end
# Same as #reorder but operates on relation in-place instead of copying.
def reorder!(*args) # :nodoc:
preprocess_order_args(args)
@@ -1071,7 +1073,7 @@ def reverse_sql_order(order_query)
end
o.split(",").map! do |s|
s.strip!
s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || s.concat(" DESC")
s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || (s << " DESC")

This comment has been minimized.

@kamipo

kamipo Oct 19, 2017

Member

Is this necessary change?

This comment has been minimized.

@matthewd

matthewd Oct 19, 2017

Member

Yeah; s can be an SqlLiteral, which implements many methods (including [I assume?] concat) for SQL construction. It's not great, but that's more Arel's fault.

I do have some vague thoughts on rearranging this method, but that's an unrelated refactoring from this PR.

This comment has been minimized.

@kamipo

kamipo Oct 19, 2017

Member

I see. Thanks for the explanation.

end
else
o
@@ -1080,6 +1082,10 @@ def reverse_sql_order(order_query)
end
def does_not_support_reverse?(order)
# Account for String subclasses like Arel::Nodes::SqlLiteral that
# override methods like #count.
order = String.new(order) unless order.instance_of?(String)
# Uses SQL function with multiple arguments.
(order.include?(",") && order.split(",").find { |section| section.count("(") != section.count(")") }) ||
# Uses "nulls first" like construction.
@@ -1113,6 +1119,12 @@ def preprocess_order_args(order_args)
klass.send(:sanitize_sql_for_order, arg)
end
order_args.flatten!
@klass.enforce_raw_sql_whitelist(
order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a },
whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
)
validate_order_args(order_args)
references = order_args.grep(String)
@@ -63,7 +63,17 @@ def sanitize_sql_for_assignment(assignments, default_table_name = table_name) #
# # => "id ASC"
def sanitize_sql_for_order(condition) # :doc:
if condition.is_a?(Array) && condition.first.to_s.include?("?")
sanitize_sql_array(condition)
enforce_raw_sql_whitelist([condition.first],
whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
)
# Ensure we aren't dealing with a subclass of String that might
# override methods we use (eg. Arel::Nodes::SqlLiteral).
if condition.first.kind_of?(String) && !condition.first.instance_of?(String)
condition = [String.new(condition.first), *condition[1..-1]]
end
Arel.sql(sanitize_sql_array(condition))
else
condition
end
@@ -427,7 +427,7 @@ def test_eager_association_loading_with_belongs_to_and_order_string_with_unquote
def test_eager_association_loading_with_belongs_to_and_order_string_with_quoted_table_name
quoted_posts_id = Comment.connection.quote_table_name("posts") + "." + Comment.connection.quote_column_name("id")
assert_nothing_raised do
Comment.includes(:post).references(:posts).order(quoted_posts_id)
Comment.includes(:post).references(:posts).order(Arel.sql(quoted_posts_id))
end
end
@@ -874,14 +874,14 @@ def test_limited_eager_with_order
posts(:thinking, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: "UPPER(posts.title)", limit: 2, offset: 1
order: Arel.sql("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: "UPPER(posts.title) DESC", limit: 2, offset: 1
order: Arel.sql("UPPER(posts.title) DESC"), limit: 2, offset: 1
).to_a
)
end
@@ -891,14 +891,14 @@ def test_limited_eager_with_multiple_order_columns
posts(:thinking, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: ["UPPER(posts.title)", "posts.id"], limit: 2, offset: 1
order: [Arel.sql("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: ["UPPER(posts.title) DESC", "posts.id"], limit: 2, offset: 1
order: [Arel.sql("UPPER(posts.title) DESC"), "posts.id"], limit: 2, offset: 1
).to_a
)
end
@@ -663,14 +663,14 @@ def test_pluck_not_auto_table_name_prefix_if_column_joined
end
def test_pluck_with_selection_clause
assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT credit_limit").sort
assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT accounts.credit_limit").sort
assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT(credit_limit)").sort
assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT credit_limit")).sort
assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT accounts.credit_limit")).sort
assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT(credit_limit)")).sort
# MySQL returns "SUM(DISTINCT(credit_limit))" as the column name unless
# an alias is provided. Without the alias, the column cannot be found
# and properly typecast.
assert_equal [50 + 53 + 55 + 60], Account.pluck("SUM(DISTINCT(credit_limit)) as credit_limit")
assert_equal [50 + 53 + 55 + 60], Account.pluck(Arel.sql("SUM(DISTINCT(credit_limit)) as credit_limit"))
end
def test_plucks_with_ids
@@ -772,7 +772,7 @@ def test_pluck_loaded_relation_sql_fragment
companies = Company.order(:name).limit(3).load
assert_queries 1 do
assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck("DISTINCT name")
assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck(Arel.sql("DISTINCT name"))
end
end
@@ -239,7 +239,7 @@ def test_exists_with_order_and_distinct
# Ensure +exists?+ runs without an error by excluding order value.
def test_exists_with_order
assert_equal true, Topic.order("invalid sql here").exists?
assert_equal true, Topic.order(Arel.sql("invalid sql here")).exists?
end
def test_exists_with_joins
@@ -652,7 +652,7 @@ def test_last_on_loaded_relation_should_not_use_sql
def test_last_with_irreversible_order
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("coalesce(author_name, title)").last
Topic.order(Arel.sql("coalesce(author_name, title)")).last
end
end
@@ -238,7 +238,7 @@ def test_finding_with_reversed_arel_assoc_order
end
def test_reverse_order_with_function
topics = Topic.order("length(title)").reverse_order
topics = Topic.order(Arel.sql("length(title)")).reverse_order
assert_equal topics(:second).title, topics.first.title
end
@@ -248,24 +248,24 @@ def test_reverse_arel_assoc_order_with_function
end
def test_reverse_order_with_function_other_predicates
topics = Topic.order("author_name, length(title), id").reverse_order
topics = Topic.order(Arel.sql("author_name, length(title), id")).reverse_order
assert_equal topics(:second).title, topics.first.title
topics = Topic.order("length(author_name), id, length(title)").reverse_order
topics = Topic.order(Arel.sql("length(author_name), id, length(title)")).reverse_order
assert_equal topics(:fifth).title, topics.first.title
end
def test_reverse_order_with_multiargument_function
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("concat(author_name, title)").reverse_order
Topic.order(Arel.sql("concat(author_name, title)")).reverse_order
end
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("concat(lower(author_name), title)").reverse_order
Topic.order(Arel.sql("concat(lower(author_name), title)")).reverse_order
end
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("concat(author_name, lower(title))").reverse_order
Topic.order(Arel.sql("concat(author_name, lower(title))")).reverse_order
end
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("concat(lower(author_name), title, length(title)").reverse_order
Topic.order(Arel.sql("concat(lower(author_name), title, length(title)")).reverse_order
end
end
@@ -277,10 +277,10 @@ def test_reverse_arel_assoc_order_with_multiargument_function
def test_reverse_order_with_nulls_first_or_last
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("title NULLS FIRST").reverse_order
Topic.order(Arel.sql("title NULLS FIRST")).reverse_order
end
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("title nulls last").reverse_order
Topic.order(Arel.sql("title nulls last")).reverse_order
end
end
@@ -373,29 +373,29 @@ def test_finding_with_order_and_take
def test_finding_with_cross_table_order_and_limit
tags = Tag.includes(:taggings).
order("tags.name asc", "taggings.taggable_id asc", "REPLACE('abc', taggings.taggable_type, taggings.taggable_type)").
order("tags.name asc", "taggings.taggable_id asc", Arel.sql("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)")).
limit(1).to_a
assert_equal 1, tags.length
end
def test_finding_with_complex_order_and_limit
tags = Tag.includes(:taggings).references(:taggings).order("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)").limit(1).to_a
tags = Tag.includes(:taggings).references(:taggings).order(Arel.sql("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)")).limit(1).to_a
assert_equal 1, tags.length
end
def test_finding_with_complex_order
tags = Tag.includes(:taggings).references(:taggings).order("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)").to_a
tags = Tag.includes(:taggings).references(:taggings).order(Arel.sql("REPLACE('abc', taggings.taggable_type, taggings.taggable_type)")).to_a
assert_equal 3, tags.length
end
def test_finding_with_sanitized_order
query = Tag.order(["field(id, ?)", [1, 3, 2]]).to_sql
query = Tag.order([Arel.sql("field(id, ?)"), [1, 3, 2]]).to_sql
assert_match(/field\(id, 1,3,2\)/, query)
query = Tag.order(["field(id, ?)", []]).to_sql
query = Tag.order([Arel.sql("field(id, ?)"), []]).to_sql
assert_match(/field\(id, NULL\)/, query)
query = Tag.order(["field(id, ?)", nil]).to_sql
query = Tag.order([Arel.sql("field(id, ?)"), nil]).to_sql
assert_match(/field\(id, NULL\)/, query)
end
@@ -1567,7 +1567,7 @@ def test_automatically_added_order_references
scope = Post.order("comments.body")
assert_equal ["comments"], scope.references_values
scope = Post.order("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")
scope = Post.order(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}"))
if current_adapter?(:OracleAdapter)
assert_equal ["COMMENTS"], scope.references_values
else
@@ -1584,15 +1584,15 @@ def test_automatically_added_order_references
scope = Post.order("comments.body asc")
assert_equal ["comments"], scope.references_values
scope = Post.order("foo(comments.body)")
scope = Post.order(Arel.sql("foo(comments.body)"))
assert_equal [], scope.references_values
end
def test_automatically_added_reorder_references
scope = Post.reorder("comments.body")
assert_equal %w(comments), scope.references_values
scope = Post.reorder("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")
scope = Post.reorder(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}"))
if current_adapter?(:OracleAdapter)
assert_equal ["COMMENTS"], scope.references_values
else
@@ -1609,7 +1609,7 @@ def test_automatically_added_reorder_references
scope = Post.reorder("comments.body asc")
assert_equal %w(comments), scope.references_values
scope = Post.reorder("foo(comments.body)")
scope = Post.reorder(Arel.sql("foo(comments.body)"))
assert_equal [], scope.references_values
end
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.