diff --git a/Appraisals b/Appraisals index 7d16c71c..b3771915 100644 --- a/Appraisals +++ b/Appraisals @@ -26,6 +26,6 @@ appraise "rails-6.1" do end appraise "rails-7.0" do - gem "rails", "~> 7.0.0" + gem "rails", "~> 7.0.3.1" gem "rails-controller-testing", "~> 1.0.5" end diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c7b93f3..3fa38f5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,25 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/). - None +## 13.0.0 (2022-08-15) + +### Breaking Changes + +- The default serializer will now use `YAML.safe_load` unless + `ActiveRecord.use_yaml_unsafe_load`. This change only affects users whose + `versions` table has `object` or `object_changes` columns of type `text`, and + who use the YAML serializer. People who use the JSON serializer, or those with + `json(b)` columns, are unaffected. Please see [doc/pt_13_yaml_safe_load.md] for + details. + +### Added + +- None + +### Fixed + +- None + ## 12.3.0 (2022-03-13) ### Breaking Changes diff --git a/doc/pt_13_yaml_safe_load.md b/doc/pt_13_yaml_safe_load.md new file mode 100644 index 00000000..abf3ea82 --- /dev/null +++ b/doc/pt_13_yaml_safe_load.md @@ -0,0 +1,49 @@ +# PT 13 uses YAML.safe_load + +Starting with 13.0.0, PT's default serializer (`PaperTrail::Serializers::YAML`) +will use `safe_load` unless `ActiveRecord.use_yaml_unsafe_load`. Earlier +versions of PT use `unsafe_load`. + +## Motivation + +> A few days ago Rails released versions 7.0.3.1, 6.1.6.1, 6.0.5.1, and 5.2.8.1. +> These are security updates that impact applications that use serialised +> attributes on Active Record models. These updates, identified by CVE-2022-32224 +> cover a possible escalation to RCE when using YAML serialised columns in Active +> Record. +> https://rubyonrails.org/2022/7/15/this-week-in-rails-rails-security-releases-improved-generator-option-handling-and-more-24774592 + +## Who is affected by this change? + +This change only affects users whose `versions` table has `object` or +`object_changes` columns of type `text`, and who use the YAML serializer. People +who use the JSON serializer, or those with `json(b)` columns, are unaffected. + +## To continue using the YAML serializer + +We recommend switching to `json(b)` columns, or at least JSON in a `text` column +(see "Other serializers" below). If you must continue using the YAML serializer, +PT users are required to configure `ActiveRecord.yaml_column_permitted_classes` +correctly for their own application. Users may want to start with the following +safe-list: + +```ruby +::ActiveRecord.use_yaml_unsafe_load = false +::ActiveRecord.yaml_column_permitted_classes = [ + ::ActiveRecord::Type::Time::Value, + ::ActiveSupport::TimeWithZone, + ::ActiveSupport::TimeZone, + ::BigDecimal, + ::Date, + ::Symbol, + ::Time +] +``` + +## Other serializers + +While YAML remains the default serializer in PT for historical compatibility, +we have recommended JSON instead, for years. See: + +- [PostgreSQL JSON column type support](https://github.com/paper-trail-gem/paper_trail/blob/v12.3.0/README.md#postgresql-json-column-type-support) +- [Convert existing YAML data to JSON](https://github.com/paper-trail-gem/paper_trail/blob/v12.3.0/README.md#convert-existing-yaml-data-to-json) diff --git a/gemfiles/rails_7.0.gemfile b/gemfiles/rails_7.0.gemfile index ed927c64..58bdd3cb 100644 --- a/gemfiles/rails_7.0.gemfile +++ b/gemfiles/rails_7.0.gemfile @@ -2,7 +2,7 @@ source "https://rubygems.org" -gem "rails", "~> 7.0.0" +gem "rails", "~> 7.0.3.1" gem "rails-controller-testing", "~> 1.0.5" gemspec path: "../" diff --git a/lib/paper_trail/serializers/yaml.rb b/lib/paper_trail/serializers/yaml.rb index 16bc5ec9..d08359ca 100644 --- a/lib/paper_trail/serializers/yaml.rb +++ b/lib/paper_trail/serializers/yaml.rb @@ -9,7 +9,17 @@ module PaperTrail extend self # makes all instance methods become module methods as well def load(string) - ::YAML.respond_to?(:unsafe_load) ? ::YAML.unsafe_load(string) : ::YAML.load(string) + if use_safe_load? + ::YAML.safe_load( + string, + permitted_classes: ::ActiveRecord.yaml_column_permitted_classes, + aliases: true + ) + elsif ::YAML.respond_to?(:unsafe_load) + ::YAML.unsafe_load(string) + else + ::YAML.load(string) + end end # @param object (Hash | HashWithIndifferentAccess) - Coming from @@ -26,6 +36,14 @@ module PaperTrail def where_object_condition(arel_field, field, value) arel_field.matches("%\n#{field}: #{value}\n%") end + + private + + # `use_yaml_unsafe_load` was added in 7.0.3.1, will be removed in 7.1.0? + def use_safe_load? + defined?(ActiveRecord.use_yaml_unsafe_load) && + !ActiveRecord.use_yaml_unsafe_load + end end end end diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 1e4a96c0..c49bfe0c 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -15,6 +15,12 @@ module PaperTrail module VersionConcern extend ::ActiveSupport::Concern + E_YAML_PERMITTED_CLASSES = <<-EOS.squish.freeze + PaperTrail encountered a Psych::DisallowedClass error during + deserialization of YAML column, indicating that + yaml_column_permitted_classes has not been configured correctly. %s + EOS + included do belongs_to :item, polymorphic: true, optional: true, inverse_of: false validates_presence_of :event @@ -348,7 +354,10 @@ module PaperTrail else begin PaperTrail.serializer.load(object_changes) - rescue StandardError # TODO: Rescue something more specific + rescue StandardError => e + if defined?(::Psych::Exception) && e.instance_of?(::Psych::Exception) + ::Kernel.warn format(E_YAML_PERMITTED_CLASSES, e) + end {} end end diff --git a/spec/dummy_app/config/application.rb b/spec/dummy_app/config/application.rb index 6f4de37d..4441c665 100644 --- a/spec/dummy_app/config/application.rb +++ b/spec/dummy_app/config/application.rb @@ -27,5 +27,19 @@ module Dummy ::Gem::Requirement.new("~> 5.2").satisfied_by?(::Rails.gem_version) config.active_record.sqlite3.represent_boolean_as_integer = true end + + # `use_yaml_unsafe_load` was added in 7.0.3.1, will be removed in 7.1.0? + if ::ActiveRecord.respond_to?(:use_yaml_unsafe_load) + ::ActiveRecord.use_yaml_unsafe_load = false + ::ActiveRecord.yaml_column_permitted_classes = [ + ::ActiveRecord::Type::Time::Value, + ::ActiveSupport::TimeWithZone, + ::ActiveSupport::TimeZone, + ::BigDecimal, + ::Date, + ::Symbol, + ::Time + ] + end end end diff --git a/spec/models/widget_spec.rb b/spec/models/widget_spec.rb index 72e2393a..d931a30c 100644 --- a/spec/models/widget_spec.rb +++ b/spec/models/widget_spec.rb @@ -46,6 +46,21 @@ RSpec.describe Widget, type: :model, versioning: true do end end + describe "#object_changes_deserialized" do + context "when the serializer raises a Psych::DisallowedClass error" do + it "prints a warning to stderr" do + allow(PaperTrail.serializer).to( + receive(:load).and_raise(::Psych::Exception, "kaboom") + ) + widget = described_class.create(name: "Henry") + ver = widget.versions.last + expect { ver.send(:object_changes_deserialized) }.to( + output(/kaboom/).to_stderr + ) + end + end + end + context "with a new record" do it "not have any previous versions" do expect(described_class.new.versions).to(eq([])) diff --git a/spec/paper_trail/serializers/yaml_spec.rb b/spec/paper_trail/serializers/yaml_spec.rb index 94aef5c6..711266ba 100644 --- a/spec/paper_trail/serializers/yaml_spec.rb +++ b/spec/paper_trail/serializers/yaml_spec.rb @@ -24,8 +24,13 @@ module PaperTrail end it "calls the expected load method based on Psych version" do + # `use_yaml_unsafe_load` was added in 7.0.3.1, will be removed in 7.1.0? + if defined?(ActiveRecord.use_yaml_unsafe_load) && !ActiveRecord.use_yaml_unsafe_load + allow(::YAML).to receive(:safe_load) + described_class.load("string") + expect(::YAML).to have_received(:safe_load) # Psych 4+ implements .unsafe_load - if ::YAML.respond_to?(:unsafe_load) + elsif ::YAML.respond_to?(:unsafe_load) allow(::YAML).to receive(:unsafe_load) described_class.load("string") expect(::YAML).to have_received(:unsafe_load) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index aaf56a92..cae5acad 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -5,7 +5,7 @@ ENV["DB"] ||= "sqlite" require "simplecov" SimpleCov.start do - add_filter "spec" + add_filter %w[Appraisals Gemfile Rakefile doc gemfiles spec] end SimpleCov.minimum_coverage(ENV["DB"] == "postgres" ? 97.3 : 92.4)