Merge pull request #1112 from RepairShopr/rails-5.2.4

Test/Fix for subquery in Rails 5.2.4

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Change Log
 
+* Fix for ActiveRecord 5.2.4 (note security fix in 5.2.4.2 for ActiveView's escape_javascript CVE-2020-5267 for all earlier versions)
+
 ## 2.3.2 - 2020-01-11
 
 * Breakfix to bump Polyamorous

CONTRIBUTING.md

@@ -64,6 +64,9 @@ Here's a quick guide:
 2. Create a thoughtfully-named branch for your changes (`git checkout -b my-new-feature`).
 
 3. Install the development dependencies by running `bundle install`.
+   To install rails other than latest (set in Gemfile): `RAILS='5-2-stable' bundle install`
+
+        $ RAILS='5-2-stable' bundle install
 
 4. Begin by running the tests. We only take pull requests with passing tests,
    and it's great to know that you have a clean slate:
@@ -84,16 +87,18 @@ Here's a quick guide:
        $ mysql -u root
        mysql> create database ransack;     
 
-   To run only the tests in a particular file: `rspec <path/to/filename>`
+   The test suite runs by default
+
+   To run only the tests in a particular file: `bundle exec rspec <path/to/filename>`
 
-        $ rspec spec/ransack/search_spec.rb
+        $ bundle exec rspec spec/ransack/search_spec.rb
 
-   To run a single test in that file: `rspec <path/to/filename> -e "test name"`
+   To run a single test in that file: `bundle exec rspec <path/to/filename> -e "test name"`
 
-        $ rspec spec/ransack/search_spec.rb -e "accepts a context option"
+        $ bundle exec rspec spec/ransack/search_spec.rb -e "accepts a context option"
 
-5. Hack away! Please use Ruby features that are compatible down to Ruby 1.9.
-   Since version 1.5, Ransack no longer maintains Ruby 1.8 compatibility.
+5. Hack away! Please use Ruby features that are compatible down to Ruby 2.3.
+   Since version 2.3.1, Ransack no longer maintains Ruby 2.2 compatibility.
 
 6. Add tests for your changes. Only refactoring and documentation changes
    require no new tests. If you are adding functionality or fixing a bug, we

Gemfile

@@ -17,7 +17,7 @@ rails_version = case rails
 gem 'faker', '~> 0.9.5'
 gem 'sqlite3', ::Gem::Version.new(rails_version) >= ::Gem::Version.new('6-0-stable') ? '~> 1.4.1' : '~> 1.3.3'
 gem 'pg', '~> 1.0'
-gem 'pry', '0.10'
+gem 'pry', '~> 0.12.2'
 gem 'byebug'
 
 case rails

lib/ransack/adapters/active_record/context.rb

@@ -172,16 +172,31 @@ def primary_key
         private
 
         def extract_correlated_key(join_root)
-          correlated_key = join_root.right.expr.left
-
-          if correlated_key.is_a? Arel::Nodes::And
-            correlated_key = correlated_key.left.left
-          elsif correlated_key.is_a? Arel::Nodes::Equality
-            correlated_key = correlated_key.left
-          elsif correlated_key.is_a? Arel::Nodes::Grouping
-            correlated_key = join_root.right.expr.right.left
+          case join_root
+          when Arel::Nodes::OuterJoin
+            # one of join_root.right/join_root.left is expected to be Arel::Nodes::On
+            if join_root.right.is_a?(Arel::Nodes::On)
+              extract_correlated_key(join_root.right.expr)
+            elsif join_root.left.is_a?(Arel::Nodes::On)
+              extract_correlated_key(join_root.left.expr)
+            else
+              raise 'Ransack encountered an unexpected arel structure'
+            end
+          when Arel::Nodes::Equality
+            pk = primary_key
+            if join_root.left == pk
+              join_root.right
+            elsif join_root.right == pk
+              join_root.left
+            else
+              nil
+            end
+          when Arel::Nodes::And
+            extract_correlated_key(join_root.left) || extract_correlated_key(join_root.right)
           else
-            correlated_key
+            # eg parent was Arel::Nodes::And and the evaluated side was one of
+            # Arel::Nodes::Grouping or MultiTenant::TenantEnforcementClause
+            nil
           end
         end
 

spec/ransack/adapters/active_record/context_spec.rb

@@ -79,6 +79,25 @@ module ActiveRecord
             expect(constraint.right.relation.name).to eql 'people'
             expect(constraint.right.name).to eql 'id'
           end
+
+          it 'build correlated subquery for multiple conditions (default scope)' do
+            search = Search.new(Person, { comments_body_not_eq: 'some_title'})
+
+            # Was
+            # SELECT "people".* FROM "people" WHERE "people"."id" NOT IN (
+            #   SELECT "comments"."disabled" FROM "comments"
+            #   WHERE "comments"."disabled" = "people"."id"
+            #     AND NOT ("comments"."body" != 'some_title')
+            # ) ORDER BY "people"."id" DESC
+            # Should Be
+            # SELECT "people".* FROM "people" WHERE "people"."id" NOT IN (
+            #   SELECT "comments"."person_id" FROM "comments"
+            #   WHERE "comments"."person_id" = "people"."id"
+            #     AND NOT ("comments"."body" != 'some_title')
+            # ) ORDER BY "people"."id" DESC
+
+            expect(search.result.to_sql).to match /.comments.\..person_id. = .people.\..id./
+          end
         end
 
         describe 'sharing context across searches' do

spec/ransack/search_spec.rb

@@ -109,6 +109,43 @@ module Ransack
         expect(s.result.to_sql).to include 'published'
       end
 
+      # The failure/oversight in Ransack::Nodes::Condition#arel_predicate or deeper is beyond my understanding of the structures
+      it 'preserves (inverts) default scope and conditions for negative subqueries' do
+        # the positive case (published_articles_title_eq) is
+        # SELECT "people".* FROM "people"
+        # LEFT OUTER JOIN "articles" ON "articles"."person_id" = "people"."id"
+        #   AND "articles"."published" = 't'
+        #   AND ('default_scope' = 'default_scope')
+        # WHERE "articles"."title" = 'Test' ORDER BY "people"."id" DESC
+        #
+        # negative case was
+        # SELECT "people".* FROM "people" WHERE "people"."id" NOT IN (
+        #   SELECT "articles"."person_id" FROM "articles"
+        #   WHERE "articles"."person_id" = "people"."id"
+        #     AND NOT ("articles"."title" != 'Test')
+        # ) ORDER BY "people"."id" DESC
+        #
+        # Should have been like
+        # SELECT "people".* FROM "people" WHERE "people"."id" NOT IN (
+        #   SELECT "articles"."person_id" FROM "articles"
+        #   WHERE "articles"."person_id" = "people"."id"
+        #     AND "articles"."title" = 'Test' AND "articles"."published" = 't' AND ('default_scope' = 'default_scope')
+        # ) ORDER BY "people"."id" DESC
+        #
+        # With tenanting (eg default_scope with column reference), NOT IN should be like
+        # SELECT "people".* FROM "people" WHERE "people"."tenant_id" = 'tenant_id' AND "people"."id" NOT IN (
+        #   SELECT "articles"."person_id" FROM "articles"
+        #   WHERE "articles"."person_id" = "people"."id"
+        #     AND "articles"."tenant_id" = 'tenant_id'
+        #     AND "articles"."title" = 'Test' AND "articles"."published" = 't' AND ('default_scope' = 'default_scope')
+        # ) ORDER BY "people"."id" DESC
+
+        pending("spec should pass, but I do not know how/where to fix lib code")
+        s = Search.new(Person, published_articles_title_not_eq: 'Test')
+        expect(s.result.to_sql).to include 'default_scope'
+        expect(s.result.to_sql).to include 'published'
+      end
+
       it 'discards empty conditions' do
         s = Search.new(Person, children_name_eq: '')
         condition = s.base[:children_name_eq]

spec/support/schema.rb

@@ -164,6 +164,8 @@ class Article < ::Article
 class Comment < ActiveRecord::Base
   belongs_to :article
   belongs_to :person
+
+  default_scope { where(disabled: false) }
 end
 
 class Tag < ActiveRecord::Base
@@ -209,6 +211,7 @@ def self.create
         t.integer  :article_id
         t.integer  :person_id
         t.text     :body
+        t.boolean  :disabled, default: false
       end
 
       create_table :tags, force: true do |t|