From 357011eb3b4e9c5fb860a34ecbb2f5cb89e89a7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Tue, 5 Apr 2022 12:08:28 +0200 Subject: [PATCH] DEV: Clean up freedom patches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch removes some of our freedom patches that have been deprecated for some time now. Some of them have been updated so we’re not shipping code based on an old version of Rails. --- app/models/topic_embed.rb | 2 +- config/initializers/003-sql_builder.rb | 3 - lib/email/sender.rb | 3 +- .../active_record_attribute_methods.rb | 7 +- lib/freedom_patches/active_record_base.rb | 57 --------- lib/freedom_patches/fast_pluck.rb | 1 - lib/freedom_patches/open_uri_redirections.rb | 100 --------------- lib/freedom_patches/safe_buffer.rb | 28 ++-- .../schema_migration_details.rb | 5 +- lib/freedom_patches/sprockets_patches.rb | 53 +++----- lib/sql_builder.rb | 121 ------------------ spec/lib/migration/column_dropper_spec.rb | 4 +- 12 files changed, 30 insertions(+), 354 deletions(-) delete mode 100644 config/initializers/003-sql_builder.rb delete mode 100644 lib/freedom_patches/active_record_base.rb delete mode 100644 lib/freedom_patches/open_uri_redirections.rb delete mode 100644 lib/sql_builder.rb diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index d3d1572c5d2..18e154e9779 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -132,7 +132,7 @@ class TopicEmbed < ActiveRecord::Base response = FetchResponse.new begin - html = uri.read(allow_redirections: :safe) + html = uri.read rescue OpenURI::HTTPError, Net::OpenTimeout return end diff --git a/config/initializers/003-sql_builder.rb b/config/initializers/003-sql_builder.rb deleted file mode 100644 index a7be88c2942..00000000000 --- a/config/initializers/003-sql_builder.rb +++ /dev/null @@ -1,3 +0,0 @@ -# frozen_string_literal: true - -require 'sql_builder' diff --git a/lib/email/sender.rb b/lib/email/sender.rb index e91adecd793..b20c69a94b8 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -466,8 +466,7 @@ module Email # via group SMTP and if reply by email site settings are configured return if !user_id || !post_id || !header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present? - # use safe variant here cause we tend to see concurrency issue - reply_key = PostReplyKey.find_or_create_by_safe!( + PostReplyKey.create_or_find_by!( post_id: post_id, user_id: user_id ).reply_key diff --git a/lib/freedom_patches/active_record_attribute_methods.rb b/lib/freedom_patches/active_record_attribute_methods.rb index 8144cba19b4..fbf07b90ab7 100644 --- a/lib/freedom_patches/active_record_attribute_methods.rb +++ b/lib/freedom_patches/active_record_attribute_methods.rb @@ -12,14 +12,9 @@ module ActiveRecord module AttributeMethods module ClassMethods - # this is for Rails 5 - def enforce_raw_sql_whitelist(*args) - nil - end - BLANK_ARRAY = [].freeze - # this patch just allows everything in Rails 6 + # this patch just allows everything in Rails 6+ def disallow_raw_sql!(args, permit: nil) # we may consider moving to https://github.com/rails/rails/pull/33330 # once all frozen string hints are in place diff --git a/lib/freedom_patches/active_record_base.rb b/lib/freedom_patches/active_record_base.rb deleted file mode 100644 index f4909ae1f80..00000000000 --- a/lib/freedom_patches/active_record_base.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -class ActiveRecord::Base - - # Handle PG::UniqueViolation as well due to concurrency - # find_or_create does find_by(hash) || create!(hash) - # in some cases find will not find and multiple creates will be called - # - # note: Rails 6 has: https://github.com/rails/rails/blob/c83e30da27eafde79164ecb376e8a28ccc8d841f/activerecord/lib/active_record/relation.rb#L171-L201 - # This means that in Rails 6 we would either use: - # - # create_or_find_by! (if we are generally creating) - # - # OR - # - # find_by(hash) || create_or_find_by(hash) (if we are generally finding) - def self.find_or_create_by_safe!(hash) - begin - find_or_create_by!(hash) - rescue PG::UniqueViolation, ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid - # try again cause another transaction could have passed by now - find_or_create_by!(hash) - end - end - - # Execute SQL manually - def self.exec_sql(*args) - - Discourse.deprecate("exec_sql should not be used anymore, please use DB.exec or DB.query instead!", drop_from: '2.9.0') - - conn = ActiveRecord::Base.connection - sql = ActiveRecord::Base.public_send(:sanitize_sql_array, args) - conn.raw_connection.async_exec(sql) - end - - def exec_sql(*args) - ActiveRecord::Base.exec_sql(*args) - end - - # Executes the given block +retries+ times (or forever, if explicitly given nil), - # catching and retrying SQL Deadlock errors. - # - # Thanks to: http://stackoverflow.com/a/7427186/165668 - # - def self.retry_lock_error(retries = 5, &block) - begin - yield - rescue ActiveRecord::StatementInvalid => e - if e.message =~ /deadlock detected/ && (retries.nil? || retries > 0) - retry_lock_error(retries ? retries - 1 : nil, &block) - else - raise e - end - end - end - -end diff --git a/lib/freedom_patches/fast_pluck.rb b/lib/freedom_patches/fast_pluck.rb index a9faa87b320..ef467c56664 100644 --- a/lib/freedom_patches/fast_pluck.rb +++ b/lib/freedom_patches/fast_pluck.rb @@ -51,7 +51,6 @@ class ActiveRecord::Relation relation = apply_join_dependency relation.pluck(*column_names) else - enforce_raw_sql_whitelist(column_names) relation = spawn relation.select_values = column_names diff --git a/lib/freedom_patches/open_uri_redirections.rb b/lib/freedom_patches/open_uri_redirections.rb deleted file mode 100644 index 5886fcc0652..00000000000 --- a/lib/freedom_patches/open_uri_redirections.rb +++ /dev/null @@ -1,100 +0,0 @@ -# frozen_string_literal: true - -##### -# Patch to allow open-uri to follow safe (http to https) -# and unsafe redirections (https to http). -# -# Original gist URL: -# https://gist.github.com/1271420 -# -# Relevant issue: -# https://redmine.ruby-lang.org/issues/3719 -# -# Source here: -# https://github.com/ruby/ruby/blob/trunk/lib/open-uri.rb -# -# Thread-safe implementation adapted from: -# https://github.com/obfusk/open_uri_w_redirect_to_https -# -# Copy and pasted from: -# https://github.com/open-uri-redirections/open_uri_redirections -# - -require "open-uri" - -module OpenURI - class << self - alias_method :open_uri_original, :open_uri - alias_method :redirectable_cautious?, :redirectable? - - def redirectable?(uri1, uri2) - case Thread.current[:__open_uri_redirections__] - when :safe - redirectable_safe? uri1, uri2 - when :all - redirectable_all? uri1, uri2 - else - redirectable_cautious? uri1, uri2 - end - end - - def redirectable_safe?(uri1, uri2) - redirectable_cautious?(uri1, uri2) || http_to_https?(uri1, uri2) - end - - def redirectable_all?(uri1, uri2) - redirectable_safe?(uri1, uri2) || https_to_http?(uri1, uri2) - end - end - - ##### - # Patches the original open_uri method, so that it accepts the - # :allow_redirections argument with these options: - # - # * :safe will allow HTTP => HTTPS redirections. - # * :all will allow HTTP => HTTPS and HTTPS => HTTP redirections. - # - # OpenURI::open can receive different kinds of arguments, like a string for - # the mode or an integer for the permissions, and then a hash with options - # like UserAgent, etc. - # - # To find the :allow_redirections option, we look for this options hash. - # - def self.open_uri(name, *rest, &block) - Thread.current[:__open_uri_redirections__] = allow_redirections(rest) - - block2 = lambda do |io| - Thread.current[:__open_uri_redirections__] = nil - block[io] - end - - begin - open_uri_original name, *rest, &(block ? block2 : nil) - ensure - Thread.current[:__open_uri_redirections__] = nil - end - end - - private - - def self.allow_redirections(args) - options = first_hash_argument(args) - options.delete :allow_redirections if options - end - - def self.first_hash_argument(arguments) - arguments.select { |arg| arg.is_a? Hash }.first - end - - def self.http_to_https?(uri1, uri2) - schemes_from([uri1, uri2]) == %w(http https) - end - - def self.https_to_http?(uri1, uri2) - schemes_from([uri1, uri2]) == %w(https http) - end - - def self.schemes_from(uris) - uris.map { |u| u.scheme.downcase } - end -end diff --git a/lib/freedom_patches/safe_buffer.rb b/lib/freedom_patches/safe_buffer.rb index 545096e0d77..5bc2feb01e4 100644 --- a/lib/freedom_patches/safe_buffer.rb +++ b/lib/freedom_patches/safe_buffer.rb @@ -5,42 +5,32 @@ # # The alternative is a broken website when this happens -class ActiveSupport::SafeBuffer - def concat(value, raise_encoding_err = false) - if !html_safe? || value.html_safe? +module FreedomPatches + module SafeBuffer + def concat(value, raise_encoding_err = false) super(value) - else - super(ERB::Util.h(value)) - end - rescue Encoding::CompatibilityError - if raise_encoding_err - raise - else + rescue Encoding::CompatibilityError + raise if raise_encoding_err encoding_diags = +"internal encoding #{Encoding.default_internal}, external encoding #{Encoding.default_external}" - if encoding != Encoding::UTF_8 encoding_diags << " my encoding is #{encoding} " - - self.force_encoding("UTF-8") + force_encoding("UTF-8") unless valid_encoding? encode!("utf-16", "utf-8", invalid: :replace) encode!("utf-8", "utf-16") end Rails.logger.warn("Encountered a non UTF-8 string in SafeBuffer - #{self} - #{encoding_diags}") end - if value.encoding != Encoding::UTF_8 - encoding_diags << " attempted to append encoding #{value.encoding} " - value = value.dup.force_encoding("UTF-8").scrub Rails.logger.warn("Attempted to concat a non UTF-8 string in SafeBuffer - #{value} - #{encoding_diags}") end - concat(value, _raise = true) end - end - alias << concat + ActiveSupport::SafeBuffer.prepend(self) + ActiveSupport::SafeBuffer.class_eval("alias << concat") + end end diff --git a/lib/freedom_patches/schema_migration_details.rb b/lib/freedom_patches/schema_migration_details.rb index 5b06a81c26a..6b72cc8fdcf 100644 --- a/lib/freedom_patches/schema_migration_details.rb +++ b/lib/freedom_patches/schema_migration_details.rb @@ -48,9 +48,6 @@ SQL rval end + ActiveRecord::Migration.prepend(self) end end - -class ActiveRecord::Migration - prepend FreedomPatches::SchemaMigrationDetails -end diff --git a/lib/freedom_patches/sprockets_patches.rb b/lib/freedom_patches/sprockets_patches.rb index eb85a23ca60..d0cb960d306 100644 --- a/lib/freedom_patches/sprockets_patches.rb +++ b/lib/freedom_patches/sprockets_patches.rb @@ -8,41 +8,8 @@ # 2. Stop using a concatenator that does tons of work checking for semicolons when # when rebuilding an asset -if Rails.env.development? || Rails.env.test? - module ActionView::Helpers::AssetUrlHelper - - def asset_path(source, options = {}) - source = source.to_s - return "" unless source.present? - return source if source =~ URI_REGEXP - - tail, source = source[/([\?#].+)$/], source.sub(/([\?#].+)$/, '') - - if extname = compute_asset_extname(source, options) - source = "#{source}#{extname}" - end - - if source[0] != ?/ - # CODE REMOVED - # source = compute_asset_path(source, options) - source = "/assets/#{source}" - end - - relative_url_root = defined?(config.relative_url_root) && config.relative_url_root - if relative_url_root - source = File.join(relative_url_root, source) unless source.starts_with?("#{relative_url_root}/") - end - - if host = compute_asset_host(source, options) - source = File.join(host, source) - end - - "#{source}#{tail}" - end - alias_method :path_to_asset, :asset_path # aliased to avoid conflicts with an asset_path named route - end - - module ::SprocketHack +module FreedomPatches + module SprocketsPatches def self.concat_javascript_sources(buf, source) if buf.bytesize > 0 # CODE REMOVED HERE @@ -51,8 +18,18 @@ if Rails.env.development? || Rails.env.test? end buf << source end + + if Rails.env.development? || Rails.env.test? + Sprockets.register_bundle_metadata_reducer 'application/javascript', :data, proc { +"" }, method(:concat_javascript_sources) + end + end +end + +if Rails.env.development? || Rails.env.test? + ActiveSupport.on_load(:action_view) do + def compute_asset_path(source, _options = {}) + "/assets/#{source}" + end + alias_method :public_compute_asset_path, :compute_asset_path end - - Sprockets.register_bundle_metadata_reducer 'application/javascript', :data, proc { +"" }, ::SprocketHack.method(:concat_javascript_sources) - end diff --git a/lib/sql_builder.rb b/lib/sql_builder.rb deleted file mode 100644 index 1517cc812ec..00000000000 --- a/lib/sql_builder.rb +++ /dev/null @@ -1,121 +0,0 @@ -# frozen_string_literal: true - -class SqlBuilder - - def initialize(template, klass = nil) - - Discourse.deprecate("SqlBuilder is deprecated and will be removed, please use DB.build instead!", drop_from: '2.9.0') - - @args = {} - @sql = template - @sections = {} - @klass = klass - end - - [:set, :where2, :where, :order_by, :limit, :left_join, :join, :offset, :select].each do |k| - define_method k do |data, args = {}| - @args.merge!(args) - @sections[k] ||= [] - @sections[k] << data - self - end - end - - def secure_category(secure_category_ids, category_alias = 'c') - if secure_category_ids.present? - where("NOT COALESCE(" << category_alias << ".read_restricted, false) OR " << category_alias << ".id IN (:secure_category_ids)", secure_category_ids: secure_category_ids) - else - where("NOT COALESCE(" << category_alias << ".read_restricted, false)") - end - self - end - - def to_sql - sql = @sql.dup - - @sections.each do |k, v| - joined = nil - case k - when :select - joined = "SELECT " << v.join(" , ") - when :where, :where2 - joined = "WHERE " << v.map { |c| "(" << c << ")" }.join(" AND ") - when :join - joined = v.map { |item| "JOIN " << item }.join("\n") - when :left_join - joined = v.map { |item| "LEFT JOIN " << item }.join("\n") - when :limit - joined = "LIMIT " << v.last.to_s - when :offset - joined = "OFFSET " << v.last.to_s - when :order_by - joined = "ORDER BY " << v.join(" , ") - when :set - joined = "SET " << v.join(" , ") - end - - sql.sub!("/*#{k}*/", joined) - end - sql - end - - def exec(args = {}) - @args.merge!(args) - - sql = to_sql - if @klass - @klass.find_by_sql(ActiveRecord::Base.public_send(:sanitize_sql_array, [sql, @args])) - else - if @args == {} - ActiveRecord::Base.exec_sql(sql) - else - ActiveRecord::Base.exec_sql(sql, @args) - end - end - end - - def self.map_exec(klass, sql, args = {}) - self.new(sql).map_exec(klass, args) - end - - class RailsDateTimeDecoder < PG::SimpleDecoder - def decode(string, tuple = nil, field = nil) - @caster ||= ActiveRecord::Type::DateTime.new - @caster.cast(string) - end - end - - class ActiveRecordTypeMap < PG::BasicTypeMapForResults - def initialize(connection) - super(connection) - rm_coder 0, 1114 - add_coder RailsDateTimeDecoder.new(name: "timestamp", oid: 1114, format: 0) - # we don't need deprecations - self.default_type_map = PG::TypeMapInRuby.new - end - end - - def self.pg_type_map - conn = ActiveRecord::Base.connection.raw_connection - @typemap ||= ActiveRecordTypeMap.new(conn) - end - - def map_exec(klass = OpenStruct, args = {}) - results = exec(args) - results.type_map = SqlBuilder.pg_type_map - - setters = results.fields.each_with_index.map do |f, index| - f.dup << "=" - end - - values = results.values - values.map! do |row| - mapped = klass.new - setters.each_with_index do |name, index| - mapped.public_send name, row[index] - end - mapped - end - end - -end diff --git a/spec/lib/migration/column_dropper_spec.rb b/spec/lib/migration/column_dropper_spec.rb index 1aa0a359b04..2684bf17688 100644 --- a/spec/lib/migration/column_dropper_spec.rb +++ b/spec/lib/migration/column_dropper_spec.rb @@ -94,8 +94,8 @@ RSpec.describe Migration::ColumnDropper do SQL expect( - ActiveRecord::Base.exec_sql("SELECT * FROM #{table_name};").values - ).to include([2, "something@email.com"]) + DB.query("SELECT * FROM #{table_name};").first.values + ).to include 2, "something@email.com" end it 'should prevent insertions to the readonly column' do