diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index fd32eaaf3a36f25eef03153f621a8eec68512ed2..21f72bb6c7bb16b1060983769e41f4e49a624304 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -159,59 +159,6 @@ def attribute_names end end - # Regexp for column names (with or without a table name prefix). Matches - # the following: - # "#{table_name}.#{column_name}" - # "#{column_name}" - COLUMN_NAME = /\A(?:\w+\.)?\w+\z/i - - # Regexp for column names with order (with or without a table name - # prefix, with or without various order modifiers). Matches the following: - # "#{table_name}.#{column_name}" - # "#{table_name}.#{column_name} #{direction}" - # "#{table_name}.#{column_name} #{direction} NULLS FIRST" - # "#{table_name}.#{column_name} NULLS LAST" - # "#{column_name}" - # "#{column_name} #{direction}" - # "#{column_name} #{direction} NULLS FIRST" - # "#{column_name} NULLS LAST" - COLUMN_NAME_WITH_ORDER = / - \A - (?:\w+\.)? - \w+ - (?:\s+asc|\s+desc)? - (?:\s+nulls\s+(?:first|last))? - \z - /ix - - def disallow_raw_sql!(args, permit: COLUMN_NAME) # :nodoc: - unexpected = nil - args.each do |arg| - next if arg.is_a?(Symbol) || Arel.arel_node?(arg) || - arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) } - (unexpected ||= []) << arg - end - - return unless unexpected - - 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.1. 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 diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb index 287753091720a8172e63fc0f3338444349ad6cef..99e1a11f30b7e6599a467fa0681386c223953ba4 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb @@ -142,6 +142,43 @@ def sanitize_as_sql_comment(value) # :nodoc: value.to_s.gsub(%r{ (/ (?: | \g<1>) \*) \+? \s* | \s* (\* (?: | \g<2>) /) }x, "") end + def column_name_matcher # :nodoc: + COLUMN_NAME + end + + def column_name_with_order_matcher # :nodoc: + COLUMN_NAME_WITH_ORDER + end + + # Regexp for column names (with or without a table name prefix). + # Matches the following: + # + # "#{table_name}.#{column_name}" + # "#{column_name}" + COLUMN_NAME = /\A(?:\w+\.)?\w+\z/i + + # Regexp for column names with order (with or without a table name prefix, + # with or without various order modifiers). Matches the following: + # + # "#{table_name}.#{column_name}" + # "#{table_name}.#{column_name} #{direction}" + # "#{table_name}.#{column_name} #{direction} NULLS FIRST" + # "#{table_name}.#{column_name} NULLS LAST" + # "#{column_name}" + # "#{column_name} #{direction}" + # "#{column_name} #{direction} NULLS FIRST" + # "#{column_name} NULLS LAST" + COLUMN_NAME_WITH_ORDER = / + \A + (?:\w+\.)? + \w+ + (?:\s+ASC|\s+DESC)? + (?:\s+NULLS\s+(?:FIRST|LAST))? + \z + /ix + + private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER + private def type_casted_binds(binds) if binds.first.is_a?(Array) diff --git a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb index 75564a61d61d097e863a0eb5d45f7ff380c552ee..84354c0187244586faedd4fa85d4dfc3ce966513 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb @@ -32,12 +32,33 @@ def quoted_binary(value) "x'#{value.hex}'" end - def _type_cast(value) - case value - when Date, Time then value - else super - end + def column_name_matcher + COLUMN_NAME + end + + def column_name_with_order_matcher + COLUMN_NAME_WITH_ORDER end + + COLUMN_NAME = /\A(?:(`?)\w+\k<1>\.)?(`?)\w+\k<2>\z/i + + COLUMN_NAME_WITH_ORDER = / + \A + (?:(`?)\w+\k<1>\.)? + (`?)\w+\k<2> + (?:\s+ASC|\s+DESC)? + \z + /ix + + private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER + + private + def _type_cast(value) + case value + when Date, Time then value + else super + end + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb index d40e0ef1f0c29a4864d3a9411a6f48467eb1e6c7..0ebed21717eeb295a9d2a38ca2d6a31649ba0252 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb @@ -78,6 +78,28 @@ def lookup_cast_type_from_column(column) # :nodoc: type_map.lookup(column.oid, column.fmod, column.sql_type) end + def column_name_matcher + COLUMN_NAME + end + + def column_name_with_order_matcher + COLUMN_NAME_WITH_ORDER + end + + COLUMN_NAME = /\A(?:("?)\w+\k<1>\.)?("?)\w+\k<2>(?:::\w+)?\z/i + + COLUMN_NAME_WITH_ORDER = / + \A + (?:("?)\w+\k<1>\.)? + ("?)\w+\k<2> + (?:::\w+)? + (?:\s+ASC|\s+DESC)? + (?:\s+NULLS\s+(?:FIRST|LAST))? + \z + /ix + + private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER + private def lookup_cast_type(sql_type) super(query_value("SELECT #{quote(sql_type)}::regtype::oid", "SCHEMA").to_i) diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb index cb9d32a577664943a1efe93e066d5ce2d46e0365..79d477cdb2d8c8da16097a4ee2763197a4fb5c10 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb @@ -45,6 +45,26 @@ def unquoted_false 0 end + def column_name_matcher + COLUMN_NAME + end + + def column_name_with_order_matcher + COLUMN_NAME_WITH_ORDER + end + + COLUMN_NAME = /\A(?:("?)\w+\k<1>\.)?("?)\w+\k<2>\z/i + + COLUMN_NAME_WITH_ORDER = / + \A + (?:("?)\w+\k<1>\.)? + ("?)\w+\k<2> + (?:\s+ASC|\s+DESC)? + \z + /ix + + private_constant :COLUMN_NAME, :COLUMN_NAME_WITH_ORDER + private def _type_cast(value) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 50ff733dc71a534e65dbd6c653a065681f4fec6f..588cb130f2cbdbbcf0cbbbdfd5ce3bc3bfec9a0f 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1254,7 +1254,7 @@ def preprocess_order_args(order_args) @klass.disallow_raw_sql!( order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a }, - permit: AttributeMethods::ClassMethods::COLUMN_NAME_WITH_ORDER + permit: connection.column_name_with_order_matcher ) validate_order_args(order_args) diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb index 750766714d179d9307399c4e6bea6fce7d52219d..5296499bad39e99dcd7030a3ffe22e3a20c8cf57 100644 --- a/activerecord/lib/active_record/sanitization.rb +++ b/activerecord/lib/active_record/sanitization.rb @@ -61,8 +61,9 @@ def sanitize_sql_for_assignment(assignments, default_table_name = table_name) # # => "id ASC" def sanitize_sql_for_order(condition) if condition.is_a?(Array) && condition.first.to_s.include?("?") - disallow_raw_sql!([condition.first], - permit: AttributeMethods::ClassMethods::COLUMN_NAME_WITH_ORDER + disallow_raw_sql!( + [condition.first], + permit: connection.column_name_with_order_matcher ) # Ensure we aren't dealing with a subclass of String that might @@ -133,6 +134,34 @@ def sanitize_sql_array(ary) end end + def disallow_raw_sql!(args, permit: connection.column_name_matcher) # :nodoc: + unexpected = nil + args.each do |arg| + next if arg.is_a?(Symbol) || Arel.arel_node?(arg) || + arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) } + (unexpected ||= []) << arg + end + + return unless unexpected + + 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.1. 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 + private def replace_bind_variables(statement, values) raise_if_bind_arity_mismatch(statement, statement.count("?"), values.size) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index f7aad9d7759b1b0914f428ec7013809f2e02f9cf..c270d60b2fad80cf1321cde4b43bebaeb861bdf8 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -523,7 +523,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(Arel.sql(quoted_posts_id)) + Comment.includes(:post).references(:posts).order(quoted_posts_id) end end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 4ec695c4c6b015d4105ab7b9f0ec0a3902faef33..91353e4f9e7169ebf972b749decabf670c829970 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1679,7 +1679,7 @@ def test_automatically_added_order_references scope = Post.order("comments.body") assert_equal ["comments"], scope.references_values - scope = Post.order(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")) + scope = Post.order("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}") if current_adapter?(:OracleAdapter) assert_equal ["COMMENTS"], scope.references_values else @@ -1704,7 +1704,7 @@ def test_automatically_added_reorder_references scope = Post.reorder("comments.body") assert_equal %w(comments), scope.references_values - scope = Post.reorder(Arel.sql("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}")) + scope = Post.reorder("#{Comment.quoted_table_name}.#{Comment.quoted_primary_key}") if current_adapter?(:OracleAdapter) assert_equal ["COMMENTS"], scope.references_values else diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb index d5d8f2a09a027be5934162127108ae303861ef7b..fc92bf73c9a1f092024e83d9f7f4f9d9e1038ab5 100644 --- a/activerecord/test/cases/unsafe_raw_sql_test.rb +++ b/activerecord/test/cases/unsafe_raw_sql_test.rb @@ -77,7 +77,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase assert_equal ids_expected, ids_disabled end - test "order: allows table and column name" do + test "order: allows table and column names" do ids_expected = Post.order(Arel.sql("title")).pluck(:id) ids_depr = with_unsafe_raw_sql_deprecated { Post.order("posts.title").pluck(:id) } @@ -87,6 +87,17 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase assert_equal ids_expected, ids_disabled end + test "order: allows quoted table and column names" do + ids_expected = Post.order(Arel.sql("title")).pluck(:id) + + quoted_title = Post.connection.quote_table_name("posts.title") + ids_depr = with_unsafe_raw_sql_deprecated { Post.order(quoted_title).pluck(:id) } + ids_disabled = with_unsafe_raw_sql_disabled { Post.order(quoted_title).pluck(:id) } + + assert_equal ids_expected, ids_depr + assert_equal ids_expected, ids_disabled + end + test "order: allows column name and direction in string" do ids_expected = Post.order(Arel.sql("title desc")).pluck(:id) @@ -116,10 +127,10 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase ["asc", "desc", ""].each do |direction| %w(first last).each do |position| - ids_expected = Post.order(Arel.sql("type #{direction} nulls #{position}")).pluck(:id) + ids_expected = Post.order(Arel.sql("type::text #{direction} nulls #{position}")).pluck(:id) - ids_depr = with_unsafe_raw_sql_deprecated { Post.order("type #{direction} nulls #{position}").pluck(:id) } - ids_disabled = with_unsafe_raw_sql_disabled { Post.order("type #{direction} nulls #{position}").pluck(:id) } + ids_depr = with_unsafe_raw_sql_deprecated { Post.order("type::text #{direction} nulls #{position}").pluck(:id) } + ids_disabled = with_unsafe_raw_sql_disabled { Post.order("type::text #{direction} nulls #{position}").pluck(:id) } assert_equal ids_expected, ids_depr assert_equal ids_expected, ids_disabled @@ -262,6 +273,17 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase assert_equal titles_expected, titles_disabled end + test "pluck: allows quoted table and column names" do + titles_expected = Post.pluck(Arel.sql("title")) + + quoted_title = Post.connection.quote_table_name("posts.title") + titles_depr = with_unsafe_raw_sql_deprecated { Post.pluck(quoted_title) } + titles_disabled = with_unsafe_raw_sql_disabled { Post.pluck(quoted_title) } + + assert_equal titles_expected, titles_depr + assert_equal titles_expected, titles_disabled + end + test "pluck: disallows invalid column name" do with_unsafe_raw_sql_disabled do assert_raises(ActiveRecord::UnknownAttributeReference) do