diff options
| -rw-r--r-- | .codeclimate.yml | 2 | ||||
| -rw-r--r-- | .rubocop.yml | 13 | ||||
| -rw-r--r-- | Gemfile | 1 | ||||
| -rw-r--r-- | Gemfile.lock | 24 | ||||
| -rw-r--r-- | actionpack/test/controller/resources_test.rb | 2 | ||||
| -rw-r--r-- | actionpack/test/dispatch/request_test.rb | 1 | ||||
| -rw-r--r-- | actionview/test/actionpack/abstract/layouts_test.rb | 2 | ||||
| -rw-r--r-- | activerecord/lib/active_record/attribute_methods.rb | 53 | ||||
| -rw-r--r-- | activerecord/lib/active_record/connection_adapters/abstract/quoting.rb | 37 | ||||
| -rw-r--r-- | activerecord/lib/active_record/connection_adapters/mysql/quoting.rb | 31 | ||||
| -rw-r--r-- | activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb | 22 | ||||
| -rw-r--r-- | activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb | 20 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/query_methods.rb | 2 | ||||
| -rw-r--r-- | activerecord/lib/active_record/sanitization.rb | 33 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/eager_test.rb | 4 | ||||
| -rw-r--r-- | activerecord/test/cases/relations_test.rb | 4 | ||||
| -rw-r--r-- | activerecord/test/cases/unsafe_raw_sql_test.rb | 30 |
17 files changed, 186 insertions, 95 deletions
diff --git a/.codeclimate.yml b/.codeclimate.yml index 7114a98266..952b330d8c 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -25,6 +25,6 @@ checks: plugins: rubocop: enabled: true - channel: rubocop-0-67 + channel: rubocop-0-71 exclude_patterns: [] diff --git a/.rubocop.yml b/.rubocop.yml index 0cfe5d5d84..22161b1e16 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,4 +1,6 @@ -require: rubocop-performance +require: + - rubocop-performance + - rubocop-rails AllCops: TargetRubyVersion: 2.5 @@ -18,9 +20,6 @@ Performance: Exclude: - '**/test/**/*' -Rails: - Enabled: true - # Prefer assert_not over assert ! Rails/AssertNot: Include: @@ -77,13 +76,13 @@ Layout/EmptyLinesAroundMethodBody: Layout/EmptyLinesAroundModuleBody: Enabled: true -Layout/FirstParameterIndentation: - Enabled: true - # Use Ruby >= 1.9 syntax for hashes. Prefer { a: :b } over { :a => :b }. Style/HashSyntax: Enabled: true +Layout/IndentFirstArgument: + Enabled: true + # Method definitions after `private` or `protected` isolated calls need one # extra level of indentation. Layout/IndentationConsistency: @@ -30,6 +30,7 @@ gem "json", ">= 2.0.0" gem "rubocop", ">= 0.47", require: false gem "rubocop-performance", require: false +gem "rubocop-rails", require: false group :doc do gem "sdoc", "~> 1.0" diff --git a/Gemfile.lock b/Gemfile.lock index 268ca008ff..39ea426ed9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -279,7 +279,6 @@ GEM image_processing (1.7.1) mini_magick (~> 4.0) ruby-vips (>= 2.0.13, < 3) - jar-dependencies (0.4.0) jaro_winkler (1.5.2) jaro_winkler (1.5.2-java) jdbc-mysql (5.1.46) @@ -349,18 +348,14 @@ GEM nokogiri (1.9.1-x86-mingw32) mini_portile2 (~> 2.4.0) os (1.0.0) - parallel (1.13.0) - parser (2.6.2.0) + parallel (1.17.0) + parser (2.6.3.0) ast (~> 2.4.0) path_expander (1.0.3) pg (1.1.3) pg (1.1.3-x64-mingw32) pg (1.1.3-x86-mingw32) psych (3.1.0) - psych (3.1.0-java) - jar-dependencies (>= 0.1.7) - psych (3.1.0-x64-mingw32) - psych (3.1.0-x86-mingw32) public_suffix (3.0.3) puma (3.12.1) puma (3.12.1-java) @@ -411,17 +406,19 @@ GEM resque (>= 1.26) rufus-scheduler (~> 3.2) retriable (3.1.2) - rubocop (0.67.2) + rubocop (0.71.0) jaro_winkler (~> 1.5.1) parallel (~> 1.10) - parser (>= 2.5, != 2.5.1.1) - psych (>= 3.1.0) + parser (>= 2.6) rainbow (>= 2.2.2, < 4.0) ruby-progressbar (~> 1.7) - unicode-display_width (>= 1.4.0, < 1.6) + unicode-display_width (>= 1.4.0, < 1.7) rubocop-performance (1.1.0) rubocop (>= 0.67.0) - ruby-progressbar (1.10.0) + rubocop-rails (2.0.0) + rack (>= 2.0) + rubocop (>= 0.70.0) + ruby-progressbar (1.10.1) ruby-vips (2.0.13) ffi (~> 1.9) ruby_dep (1.5.0) @@ -499,7 +496,7 @@ GEM uber (0.1.0) uglifier (4.1.19) execjs (>= 0.3.0, < 3) - unicode-display_width (1.5.0) + unicode-display_width (1.6.0) useragent (0.16.10) vegas (0.1.11) rack (>= 1.0.0) @@ -584,6 +581,7 @@ DEPENDENCIES resque-scheduler rubocop (>= 0.47) rubocop-performance + rubocop-rails sass-rails sdoc (~> 1.0) selenium-webdriver (>= 3.5.0) diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb index d2146f12a5..1e42a3a90d 100644 --- a/actionpack/test/controller/resources_test.rb +++ b/actionpack/test/controller/resources_test.rb @@ -36,7 +36,6 @@ class ResourcesTest < ActionController::TestCase collection: collection_methods, member: member_methods, path_names: path_names do - assert_restful_routes_for :messages, collection: collection_methods, member: member_methods, @@ -58,7 +57,6 @@ class ResourcesTest < ActionController::TestCase collection: collection_methods, member: member_methods, path_names: path_names do |options| - collection_methods.each_key do |action| assert_named_route "/messages/#{path_names[action] || action}", "#{action}_messages_path", action: action end diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index eb49396145..0ec8dd25e0 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -681,7 +681,6 @@ end class RequestMethod < BaseRequestTest test "method returns environment's request method when it has not been overridden by middleware".squish do - ActionDispatch::Request::HTTP_METHODS.each do |method| request = stub_request("REQUEST_METHOD" => method) diff --git a/actionview/test/actionpack/abstract/layouts_test.rb b/actionview/test/actionpack/abstract/layouts_test.rb index 1146e6f64b..72d8e54bf8 100644 --- a/actionview/test/actionpack/abstract/layouts_test.rb +++ b/actionview/test/actionpack/abstract/layouts_test.rb @@ -295,7 +295,7 @@ module AbstractControllerTests 10.times do |x| controller = WithString.new controller.define_singleton_method :index do - render template: ActionView::Template::Text.new("Hello string!"), locals: { :"x#{x}" => :omg } + render template: ActionView::Template::Text.new("Hello string!"), locals: { "x#{x}": :omg } end controller.process(:index) end diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index fd32eaaf3a..21f72bb6c7 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -159,59 +159,6 @@ module ActiveRecord 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 2877530917..99e1a11f30 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 @@ module ActiveRecord 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 75564a61d6..84354c0187 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 @@ module ActiveRecord "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 d40e0ef1f0..0ebed21717 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 @@ module ActiveRecord 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 cb9d32a577..79d477cdb2 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 @@ module ActiveRecord 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 50ff733dc7..588cb130f2 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1254,7 +1254,7 @@ module ActiveRecord @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 750766714d..5296499bad 100644 --- a/activerecord/lib/active_record/sanitization.rb +++ b/activerecord/lib/active_record/sanitization.rb @@ -61,8 +61,9 @@ module ActiveRecord # # => "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 @@ module ActiveRecord 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 f7aad9d775..38723b6c19 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -523,7 +523,7 @@ class EagerAssociationTest < ActiveRecord::TestCase 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 @@ -789,7 +789,6 @@ class EagerAssociationTest < ActiveRecord::TestCase .where("comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'") .references(:comments) .scoping do - posts = authors(:david).posts.limit(2).to_a assert_equal 2, posts.size end @@ -798,7 +797,6 @@ class EagerAssociationTest < ActiveRecord::TestCase .where("authors.name = 'David' AND (comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment')") .references(:authors, :comments) .scoping do - count = Post.limit(2).count assert_equal count, posts.size end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 4ec695c4c6..91353e4f9e 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1679,7 +1679,7 @@ class RelationTest < ActiveRecord::TestCase 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 @@ class RelationTest < ActiveRecord::TestCase 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 d5d8f2a09a..fc92bf73c9 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 |