Merge pull request #697 from airblade/fix_issue_696_sqli

Bind parameters in where_object, where_object_changes

.rubocop.yml

@@ -17,6 +17,11 @@ AllCops:
 Style/AlignParameters:
   EnforcedStyle: with_fixed_indentation
 
+# Please use semantic style, e.g. `do` when there's a side-effect, else `{}`.
+# The semantic style is too nuanced to lint, so the cop is disabled.
+Style/BlockDelimiters:
+  Enabled: false
+
 Style/DotPosition:
   EnforcedStyle: trailing
 

.rubocop_todo.yml

@@ -83,15 +83,6 @@ Style/AndOr:
   Exclude:
     - 'lib/paper_trail/has_paper_trail.rb'
 
-# Offense count: 5
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, SupportedStyles, ProceduralMethods, FunctionalMethods, IgnoredMethods.
-Style/BlockDelimiters:
-  Exclude:
-    - 'doc/bug_report_template.rb'
-    - 'lib/paper_trail/has_paper_trail.rb'
-    - 'lib/paper_trail/reifier.rb'
-
 # Offense count: 1
 # Cop supports --auto-correct.
 # Configuration parameters: EnforcedStyle, SupportedStyles.

CONTRIBUTING.md

@@ -76,12 +76,12 @@ DB=postgres bundle exec rake prepare
 # If this is the first test run ever, create databases.
 # Unlike mysql, use create/migrate instead of setup.
 cd test/dummy
-RAILS_ENV=test bundle exec rake db:create
-RAILS_ENV=test bundle exec rake db:migrate
-RAILS_ENV=foo bundle exec rake db:create
-RAILS_ENV=foo bundle exec rake db:migrate
-RAILS_ENV=bar bundle exec rake db:create
-RAILS_ENV=bar bundle exec rake db:migrate
+DB=postgres RAILS_ENV=test bundle exec rake db:create
+DB=postgres RAILS_ENV=test bundle exec rake db:migrate
+DB=postgres RAILS_ENV=foo bundle exec rake db:create
+DB=postgres RAILS_ENV=foo bundle exec rake db:migrate
+DB=postgres RAILS_ENV=bar bundle exec rake db:create
+DB=postgres RAILS_ENV=bar bundle exec rake db:migrate
 cd ../..
 
 # Run tests

lib/paper_trail/version_concern.rb

@@ -110,48 +110,49 @@ def where_object(args = {})
         raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash)
 
         if columns_hash['object'].type == :jsonb
-          where_conditions = "object @> '#{args.to_json}'::jsonb"
+          where("object @> ?", args.to_json)
         elsif columns_hash['object'].type == :json
-          where_conditions = args.map do |field, value|
-            "object->>'#{field}' = '#{value}'"
+          predicates = []
+          values = []
+          args.each do |field, value|
+            predicates.push "object->>? = ?"
+            values.concat([field, value.to_s])
           end
-          where_conditions = where_conditions.join(" AND ")
+          sql = predicates.join(" and ")
+          where(sql, *values)
         else
           arel_field = arel_table[:object]
-
-          where_conditions = args.map do |field, value|
+          where_conditions = args.map { |field, value|
             PaperTrail.serializer.where_object_condition(arel_field, field, value)
-          end.reduce do |condition1, condition2|
-            condition1.and(condition2)
-          end
+          }.reduce { |a, e| a.and(e) }
+          where(where_conditions)
         end
-
-        where(where_conditions)
       end
 
       def where_object_changes(args = {})
         raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash)
 
         if columns_hash['object_changes'].type == :jsonb
           args.each { |field, value| args[field] = [value] }
-          where_conditions = "object_changes @> '#{args.to_json}'::jsonb"
+          where("object_changes @> ?", args.to_json)
         elsif columns_hash['object'].type == :json
-          where_conditions = args.map do |field, value|
-            "((object_changes->>'#{field}' ILIKE '[#{value.to_json},%') " +
-              "OR (object_changes->>'#{field}' ILIKE '[%,#{value.to_json}]%'))"
+          predicates = []
+          values = []
+          args.each do |field, value|
+            predicates.push(
+              "((object_changes->>? ILIKE ?) OR (object_changes->>? ILIKE ?))"
+            )
+            values.concat([field, "[#{value.to_json},%", field, "[%,#{value.to_json}]%"])
           end
-          where_conditions = where_conditions.join(" AND ")
+          sql = predicates.join(" and ")
+          where(sql, *values)
         else
           arel_field = arel_table[:object_changes]
-
-          where_conditions = args.map do |field, value|
+          where_conditions = args.map { |field, value|
             PaperTrail.serializer.where_object_changes_condition(arel_field, field, value)
-          end.reduce do |condition1, condition2|
-            condition1.and(condition2)
-          end
+          }.reduce { |a, e| a.and(e) }
+          where(where_conditions)
         end
-
-        where(where_conditions)
       end
 
       def primary_key_is_int?

spec/models/json_version_spec.rb

@@ -15,6 +15,16 @@
         describe '#where_object' do
           it { expect(JsonVersion).to respond_to(:where_object) }
 
+          it "escapes values" do
+            f = Fruit.create(:name => 'Bobby')
+            expect(
+              f.
+                versions.
+                where_object(:name => "Robert'; DROP TABLE Students;--").
+                count
+            ).to eq(0)
+          end
+
           context "invalid arguments" do
             it "should raise an error" do
               expect { JsonVersion.where_object(:foo) }.to raise_error(ArgumentError)
@@ -44,6 +54,16 @@
         describe '#where_object_changes' do
           it { expect(JsonVersion).to respond_to(:where_object_changes) }
 
+          it "escapes values" do
+            f = Fruit.create(:name => 'Bobby')
+            expect(
+              f.
+                versions.
+                where_object_changes(:name => "Robert'; DROP TABLE Students;--").
+                count
+            ).to eq(0)
+          end
+
           context "invalid arguments" do
             it "should raise an error" do
               expect { JsonVersion.where_object_changes(:foo) }.to raise_error(ArgumentError)