ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

remove SafeYAML 

Psych has safe_load now, and it's fairly trivial to convert our existing
overrides to use that instead

Change-Id: I2648df8d4574e15fc9072a25882e318d902765c3
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271939
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
This commit is contained in:
Cody Cutrer 2021-08-20 15:34:14 -06:00
parent af0b60637b
commit 31c7af578f
12 changed files with 173 additions and 298 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Dockerfile.jenkins-cache
View File

@ -109,7 +109,6 @@ RUN --mount=target=/tmp/src \
gems/plugins/.i18n* \
lib/brandable_css.rb \
lib/canvas_logger.rb \
lib/canvas_yaml.rb \
lib/logging_filter.rb \
lib/request_cache.rb \
lib/temp_cache.rb \

1
Gemfile.d/app.rb
View File

@ -130,7 +130,6 @@ gem 'ruby-duration', '3.2.3', require: false
gem 'ruby2_keywords', '0.0.3'
gem 'rubycas-client', '2.3.9', require: false
gem 'rubyzip', '2.3.0', require: 'zip'
gem 'safe_yaml', '1.0.5', require: false
gem 'saml2', '3.1.0'
gem 'nokogiri-xmlsec-instructure', '0.10.1', require: false
gem 'sanitize', '5.2.3', require: false

10
Gemfile.d/test.rb
View File

@ -55,7 +55,7 @@ group :test do
gem 'testrailtagging', '0.3.8.7', require: false
gem 'webmock', '3.8.2', require: false
gem 'crack', '0.4.3', require: false
gem 'crack', '0.4.5', require: false
gem 'timecop', '0.9.1'
gem 'jira_ref_parser', '1.0.1'
gem 'headless', '2.3.1', require: false
@ -70,8 +70,8 @@ group :test do
gem 'flakey_spec_catcher', require: false
gem 'factory_bot', '6.1.0', require: false
gem 'rspec_junit_formatter', require: false
gem 'axe-core-selenium', '4.1.0', require: false
gem 'axe-core-rspec', '4.1.0', require: false
gem 'axe-core-api', '4.1.0', require:false
gem 'stormbreaker', '0.0.3', require: false
gem 'axe-core-selenium', '4.2.1', require: false
gem 'axe-core-rspec', '4.2.1', require: false
gem 'axe-core-api', '4.2.1', require:false
gem 'stormbreaker', '0.0.4', require: false
end

31
config/application.rb
View File

@ -17,18 +17,8 @@
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
# Put this in config/application.rb
require File.expand_path('../boot', __FILE__)
require_relative 'boot'
require_relative '../lib/canvas_yaml'
# Yes, it doesn't seem DRY to list these both in the if and else
# but this used to be "require 'rails/all'" which included sprockets.
# I needed to explicitly opt-out of sprockets but since I'm not sure
# about the other frameworks, I left this so it would be exactly the same
# as "require 'rails/all'" but without sprockets--even though it is a little
# different then the rails 3 else block. If the difference is not intended,
# they can be pulled out of the if/else
require "active_record/railtie"
require "action_controller/railtie"
require "action_mailer/railtie"
@ -248,25 +238,6 @@ module CanvasRails
Autoextend.hook(:"ActiveRecord::ConnectionAdapters::PostgreSQL::OID::TypeMapInitializer",
TypeMapInitializerExtensions,
method: :prepend)
SafeYAML.singleton_class.send(:attr_accessor, :safe_parsing)
module SafeYAMLWithFlag
def load(*args)
previous, self.safe_parsing = safe_parsing, true
super
ensure
self.safe_parsing = previous
end
end
SafeYAML.singleton_class.prepend(SafeYAMLWithFlag)
Psych.add_domain_type("ruby/object", "Class") do |_type, val|
if SafeYAML.safe_parsing && !Canvas::Migration.valid_converter_classes.include?(val)
raise "Cannot load class #{val} from YAML"
end
val.constantize
end
module PatchThorWarning
# active_model_serializers should be passing `type: :boolean` here:
# https://github.com/rails-api/active_model_serializers/blob/v0.9.0.alpha1/lib/active_model/serializer/generators/serializer/scaffold_controller_generator.rb#L10

98
config/initializers/safe_yaml.rb Normal file
View File

@ -0,0 +1,98 @@
# frozen_string_literal: true
#
# Copyright (C) 2015 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
require 'yaml'
require 'date'
# SafeYAML-like interface, but vanilla Psych
module SafeYAML
class << self
attr_accessor :permitted_classes
def whitelist_class!(*klasses)
permitted_classes.concat(klasses).uniq!
end
end
self.permitted_classes = []
whitelist_class!(
ActiveSupport::HashWithIndifferentAccess,
ActiveSupport::TimeWithZone,
ActiveSupport::TimeZone,
ActionController::Parameters,
BigDecimal,
Date,
DateTime,
Mime::Type,
Mime::NullType,
OpenObject,
OpenStruct,
Symbol,
Time,
URI::HTTP,
URI::HTTPS,
)
module Psych
# load defaults to safe
def load(*args, safe: true, **kwargs)
return super(*args, **kwargs) unless safe
safe_load(*args, **kwargs)
end
def unsafe_load(*args, **kwargs)
load(*args, safe: false, **kwargs)
end
def safe_load(yaml, permitted_classes: [], **kwargs)
super(yaml, permitted_classes: permitted_classes + SafeYAML.permitted_classes, aliases: true, **kwargs)
end
end
end
Psych.singleton_class.prepend(SafeYAML::Psych)
module ScalarScannerFix
# in rubies < 2.7, Psych uses a regex to identify an integer, then strips commas and underscores,
# then checks _again_ against the regex. In 2.7, the second check was eliminated because the
# stripping was inlined in the name of optimization to avoid a string allocation. unfortunately
# this means something like 0x_ which passes the first check is not a valid number, and will
# throw an exception. this is the simplest way to catch that case without completely reverting
# the optimization
def parse_int(string)
super
rescue ArgumentError
string
end
end
Psych::ScalarScanner.prepend(ScalarScannerFix)
module YAMLSingletonFix
def revive(klass, node)
if klass < Singleton
klass.instance
elsif klass == Set
super.tap{|s| s.instance_variable_get(:@hash).default = false}
else
super
end
end
end
Psych::Visitors::ToRuby.prepend(YAMLSingletonFix)

2
gems/canvas_security/lib/canvas_security.rb
View File

@ -79,7 +79,7 @@ module CanvasSecurity
@config ||= begin
path = Rails.root + 'config/security.yml'
raise('config/security.yml missing, see security.yml.example') unless File.exist?(path)
YAML.safe_load(ERB.new(File.read(path)).result)[Rails.env]
YAML.safe_load(ERB.new(File.read(path)).result, aliases: true)[Rails.env]
end
end

2
gems/config_file/lib/config_file.rb
View File

@ -44,7 +44,7 @@ module ConfigFile
path = Rails.root.join('config', "#{config_name}.yml")
if File.exist?(path)
config_string = ERB.new(File.read(path))
config = YAML.load(config_string.result)
config = YAML.safe_load(config_string.result, aliases: true)
config = config.with_indifferent_access if config.respond_to?(:with_indifferent_access)
end
if config

5
lib/canvas/migration.rb
View File

@ -30,13 +30,10 @@ module Canvas::Migration
def self.logger
Rails.logger
end
def logger
Rails.logger
end
def self.valid_converter_classes
@converter_classes ||= Canvas::Plugin.all_for_tag(:export_system).map {|p| p.meta["settings"]["provides"].try(:values) }.flatten.compact.uniq.map(&:name)
end
end
require_dependency 'canvas/migration/migrator'

188
lib/canvas_yaml.rb
View File

@ -1,188 +0,0 @@
# frozen_string_literal: true
#
# Copyright (C) 2015 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
# This is an initializer, but needs to be required earlier in the load process,
# and before canvas-jobs
# We need to make sure that safe_yaml is loaded *after* the YAML engine
# is switched to Psych. Otherwise we
# won't have access to (safe|unsafe)_load.
require 'yaml'
require 'date' if RUBY_VERSION >= "2.5.0"
require 'safe_yaml'
module FixSafeYAMLNullMerge
def merge_into_hash(hash, array)
return unless array
super
end
end
SafeYAML::Resolver.prepend(FixSafeYAMLNullMerge)
SafeYAML::OPTIONS.merge!(
default_mode: :safe,
deserialize_symbols: true,
raise_on_unknown_tag: true,
# This tag whitelist is syck specific. We'll need to tweak it when we upgrade to psych.
# See the tests in spec/lib/safe_yaml_spec.rb
whitelisted_tags: %w[
!ruby/sym
!ruby/symbol
!binary
!float
!float#exp
!float#inf
!str
tag:yaml.org,2002:str
!timestamp
!timestamp#iso8601
!timestamp#spaced
!map:HashWithIndifferentAccess
!map:ActiveSupport::HashWithIndifferentAccess
!map:WeakParameters
!ruby/hash:HashWithIndifferentAccess
!ruby/hash:ActiveSupport::HashWithIndifferentAccess
!ruby/hash:WeakParameters
!ruby/hash:ActionController::Parameters
!ruby/object:Class
!ruby/object:OpenStruct
!ruby/object:Mime::Type
!ruby/object:Mime::NullType
!ruby/object:URI::HTTP
!ruby/object:URI::HTTPS
!ruby/object:OpenObject
!ruby/object:DateTime
!ruby/object:BigDecimal
!ruby/object:ActiveSupport::TimeWithZone
!ruby/object:ActiveSupport::TimeZone
],
)
module Syckness
TAG = "#GETDOWNWITHTHESYCKNESS\n"
end
SafeYAML::PsychResolver.class_eval do
attr_accessor :aliased_nodes
end
module AddClassWhitelist
SafeYAML::OPTIONS[:whitelisted_classes] ||= []
# This isn't really a bang method but it has been included here to maintain
# consistency with SafeYAML's whitelist! methods
def whitelist_classes!(*constants)
constants.each do |const|
whitelist_constant!(const)
end
end
def whitelist_class!(const)
const_name = const.name
raise "#{const} cannont be anonymous" unless const_name.present?
SafeYAML::OPTIONS[:whitelisted_classes] << const_name
end
end
SafeYAML.singleton_class.prepend(AddClassWhitelist)
module MaintainAliases
def accept(node)
if node.respond_to?(:anchor) && node.anchor && @resolver.get_node_type(node) != :alias
@resolver.aliased_nodes[node.anchor] = node
end
super
end
end
SafeYAML::SafeToRubyVisitor.prepend(MaintainAliases)
module AcceptClasses
def accept(node)
if node.tag && node.tag == '!ruby/class'
val = node.value
if @resolver.options[:whitelisted_classes].include?(val)
val.constantize
else
raise "YAML deserialization of constant not allowed: #{val}"
end
else
super
end
end
end
SafeYAML::SafeToRubyVisitor.prepend(AcceptClasses)
module ResolveClasses
def resolve_scalar(node)
if node.tag && node.tag == '!ruby/class'
val = node.value
if options[:whitelisted_classes].include?(val)
val.constantize
else
raise "YAML deserialization of constant not allowed: #{val}"
end
else
super
end
end
end
SafeYAML::Resolver.prepend(ResolveClasses)
module ScalarScannerFix
# in rubies < 2.7, Psych uses a regex to identify an integer, then strips commas and underscores,
# then checks _again_ against the regex. In 2.7, the second check was eliminated because the
# stripping was inlined in the name of optimization to avoid a string allocation. unfortunately
# this means something like 0x_ which passes the first check is not a valid number, and will
# throw an exception. this is the simplest way to catch that case without completely reverting
# the optimization
def parse_int(string)
super
rescue ArgumentError
string
end
end
Psych::ScalarScanner.prepend(ScalarScannerFix)
module ScalarTransformFix
def to_guessed_type(value, quoted=false, options=nil)
return value if quoted
if value.is_a?(String)
@ss ||= Psych::ScalarScanner.new(Psych::ClassLoader.new)
return @ss.tokenize(value) # just skip straight to Psych if it's a scalar because SafeYAML's transform mades me a sad panda
end
value
end
end
SafeYAML::Transform.singleton_class.prepend(ScalarTransformFix)
module YAMLSingletonFix
def revive(klass, node)
if klass < Singleton
klass.instance
elsif klass == Set
super.tap{|s| s.instance_variable_get(:@hash).default = false}
else
super
end
end
end
Psych::Visitors::ToRuby.prepend(YAMLSingletonFix)

4
lib/microsoft_sync/state_machine_job.rb
View File

@ -397,14 +397,14 @@ module MicrosoftSync
# Find a delayed job on the strand with arguments that match the selector
# IMPORTANT: To avoid unnecessary database loads of the state_record
# object, this uses SafeYAML to avoid instantiating objects in the YAML;
# object, this uses YAML to avoid instantiating objects in the YAML;
# this also means that the args passed into the selector will be only Ruby
# primitives. So if you use non-primitives in initial_mem_state, duplicate
# job detection won't work.
def find_delayed_job(strand, &args_selector)
Delayed::Job.where(strand: strand).find_each.find do |job|
job != Delayed::Worker.current_job && args_selector[
SafeYAML.load(job.handler, nil, raise_on_unknown_tag: false)['args']
YAML.unsafe_load(job.handler)['args']
]
end
end

1
script/delayed_job
View File

@ -6,7 +6,6 @@ ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../../Gemfile', __FILE__)
# We avoid loading the whole rails environment here, so that commands like
# `status` and `start` return much faster.
require 'bundler/setup'
require_relative '../lib/canvas_yaml'
require 'delayed_job'
Delayed::CLI.new.run()

128
spec/lib/safe_yaml_spec.rb
View File

@ -21,80 +21,80 @@
require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
describe "safe_yaml" do
let(:test_yaml) {
yaml = <<-YAML
---
hwia: !map:HashWithIndifferentAccess
a: 1
b: 2
float: !float
5.1
float_with_exp: -1.7763568394002505e-15
float_inf: .inf
os: !ruby/object:OpenStruct
modifiable: true
table:
:a: 1
:b: 2
:sub: !ruby/object:OpenStruct
modifiable: true
table:
:c: 3
str: !str
hai
mime: !ruby/object:Mime::Type
string: png
symbol:
synonyms: []
http: !ruby/object:URI::HTTP
fragment:
host: example.com
opaque:
parser:
password:
path: /
port: 80
query:
registry:
scheme: http
user:
https: !ruby/object:URI::HTTPS
fragment:
host: example.com
opaque:
parser:
password:
path: /
port: 443
query:
registry:
scheme: https
user:
ab: !ruby/object:Class AcademicBenchmark::Converter
qt: !ruby/object:Class Qti::Converter
verbose_symbol: !ruby/symbol blah
oo: !ruby/object:OpenObject
table:
:a: 1
let(:test_yaml) do
<<~YAML
---
hwia: !map:HashWithIndifferentAccess
a: 1
b: 2
float: !float
5.1
float_with_exp: -1.7763568394002505e-15
float_inf: .inf
os: !ruby/object:OpenStruct
modifiable: true
table:
:a: 1
:b: 2
:sub: !ruby/object:OpenStruct
modifiable: true
table:
:c: 3
str: !str
hai
mime: !ruby/object:Mime::Type
string: png
symbol:
synonyms: []
http: !ruby/object:URI::HTTP
fragment:
host: example.com
opaque:
parser:
password:
path: /
port: 80
query:
registry:
scheme: http
user:
https: !ruby/object:URI::HTTPS
fragment:
host: example.com
opaque:
parser:
password:
path: /
port: 443
query:
registry:
scheme: https
user:
ab: !ruby/object:Class AcademicBenchmark::Converter
qt: !ruby/object:Class Qti::Converter
verbose_symbol: !ruby/symbol blah
oo: !ruby/object:OpenObject
table:
:a: 1
YAML
}
end
it "should be used by default" do
yaml = <<-YAML
--- !ruby/object:ActionController::Base
real_format:
YAML
expect { YAML.load yaml }.to raise_error("Unknown YAML tag '!ruby/object:ActionController::Base'")
yaml = <<~YAML
--- !ruby/regexp /regex/
YAML
expect { YAML.load yaml }.to raise_error(Psych::DisallowedClass)
result = YAML.unsafe_load yaml
expect(result.class).to eq ActionController::Base
expect(result.class).to eq Regexp
end
it "doesn't allow deserialization of arbitrary classes" do
expect { YAML.load(YAML.dump(ActionController::Base)) }.to raise_error("YAML deserialization of constant not allowed: ActionController::Base")
expect { YAML.load(YAML.dump(/regex/)) }.to raise_error(Psych::DisallowedClass)
end
it "allows deserialization of arbitrary classes when unsafe_loading" do
expect(YAML.unsafe_load(YAML.dump(ActionController::Base))).to eq ActionController::Base
regex = /regex/
expect(YAML.unsafe_load(YAML.dump(regex))).to eq regex
end
it "should allow some whitelisted classes" do