From eea67c1774a7d0098037a9cade59a118d2a78f7a Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 22 Jul 2021 14:01:34 +0000 Subject: [PATCH] Performance: Added mysql helper to do strategy: reset manually for mysql. --- config/brakeman.ignore | 86 ++++++++++++++++---------------- lib/mysql_strategy.rb | 73 +++++++++++++++++++++++++++ lib/tasks/zammad/db/rebuild.rake | 16 ++++++ lib/tasks/zammad/db/reset.rake | 8 +-- spec/support/db_strategies.rb | 9 ++-- 5 files changed, 137 insertions(+), 55 deletions(-) create mode 100644 lib/mysql_strategy.rb create mode 100644 lib/tasks/zammad/db/rebuild.rake diff --git a/config/brakeman.ignore b/config/brakeman.ignore index f27e7ab70..dda7bfbd7 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -460,46 +460,6 @@ "confidence": "Medium", "note": "Admin configured RegExp" }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "a5818edfcce4a3860c36ce71d434d1d4dd91fe3cac9ac945c71e4e2932ffe6cc", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/ticket/search.rb", - "line": 203, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "Ticket.select(\"DISTINCT(tickets.id), #{::SqlHelper.new(:object => (self)).get_order_select(::SqlHelper.new(:object => (self)).get_sort_by(params, \"updated_at\"), ::SqlHelper.new(:object => (self)).get_order_by(params, \"desc\"), \"tickets.updated_at\")}\")", - "render_path": null, - "location": { - "type": "method", - "class": "Ticket::Search", - "method": "search" - }, - "user_input": "::SqlHelper.new(:object => (self)).get_order_select(::SqlHelper.new(:object => (self)).get_sort_by(params, \"updated_at\"), ::SqlHelper.new(:object => (self)).get_order_by(params, \"desc\"), \"tickets.updated_at\")", - "confidence": "Medium", - "note": "SqlHelper does properly escape table and column names." - }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "a5818edfcce4a3860c36ce71d434d1d4dd91fe3cac9ac945c71e4e2932ffe6cc", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/ticket/search.rb", - "line": 212, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "Ticket.select(\"DISTINCT(tickets.id), #{::SqlHelper.new(:object => (self)).get_order_select(::SqlHelper.new(:object => (self)).get_sort_by(params, \"updated_at\"), ::SqlHelper.new(:object => (self)).get_order_by(params, \"desc\"), \"tickets.updated_at\")}\")", - "render_path": null, - "location": { - "type": "method", - "class": "Ticket::Search", - "method": "search" - }, - "user_input": "::SqlHelper.new(:object => (self)).get_order_select(::SqlHelper.new(:object => (self)).get_sort_by(params, \"updated_at\"), ::SqlHelper.new(:object => (self)).get_order_by(params, \"desc\"), \"tickets.updated_at\")", - "confidence": "Medium", - "note": "SqlHelper does properly escape table and column names." - }, { "warning_type": "Dangerous Send", "warning_code": 23, @@ -540,6 +500,26 @@ "confidence": "Medium", "note": "ObjectLookup.by_id works as designed" }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "be422b13e9cd280bc5ae570cd575777a4d48d8a53aed09bb59d1db85eee4927b", + "check_name": "Execute", + "message": "Possible command injection", + "file": "lib/mysql_strategy.rb", + "line": 64, + "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", + "code": "system(\"mysqldump #{mysql_arguments} > #{backup_file}\", :exception => true)", + "render_path": null, + "location": { + "type": "method", + "class": "MysqlStrategy", + "method": "s(:self).backup" + }, + "user_input": "mysql_arguments", + "confidence": "Medium", + "note": "Mysql arguments are internal / from config." + }, { "warning_type": "Denial of Service", "warning_code": 76, @@ -716,6 +696,26 @@ "confidence": "Weak", "note": "Reflections come from the models themselves and are thus safe to use." }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "fe15417756eed2c518c355309ee042b57df5f88a5410858dce3fa9fe9c893b84", + "check_name": "Execute", + "message": "Possible command injection", + "file": "lib/mysql_strategy.rb", + "line": 56, + "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", + "code": "system(\"mysql #{mysql_arguments} < #{backup_file}\", :exception => true)", + "render_path": null, + "location": { + "type": "method", + "class": "MysqlStrategy", + "method": "s(:self).rollback" + }, + "user_input": "mysql_arguments", + "confidence": "Medium", + "note": "Mysql arguments are internal / from config." + }, { "warning_type": "Denial of Service", "warning_code": 76, @@ -723,7 +723,7 @@ "check_name": "RegexDoS", "message": "Model attribute used in regular expression", "file": "app/models/ticket.rb", - "line": 1612, + "line": 1577, "link": "https://brakemanscanner.org/docs/warning_types/denial_of_service/", "code": "/#{Setting.get(\"send_no_auto_response_reg_exp\")}/i", "render_path": null, @@ -737,6 +737,6 @@ "note": "Admin configured RegExp" } ], - "updated": "2021-07-20 13:22:48 +0200", - "brakeman_version": "5.0.4" + "updated": "2021-07-22 13:52:48 +0200", + "brakeman_version": "5.1.1" } diff --git a/lib/mysql_strategy.rb b/lib/mysql_strategy.rb new file mode 100644 index 000000000..69fe0e10d --- /dev/null +++ b/lib/mysql_strategy.rb @@ -0,0 +1,73 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'fileutils' +require 'digest' + +class MysqlStrategy + def self.db? + ActiveRecord::Base.connection.instance_values['config'][:adapter] == 'mysql2' + end + + def self.db_checksum + @@db_checksum ||= begin # rubocop:disable Style/ClassVars + files = Dir[ Rails.root.join('db/**/*') ].reject { |f| File.directory?(f) } + content = files.map { |f| File.read(f) }.join + Digest::MD5.hexdigest(content).to_s + end + end + + def self.basepath + Rails.root.join("tmp/mysql_reset/#{db_checksum}/") + end + + def self.backup_file + "#{basepath}db.sql" + end + + def self.backup_exists? + File.exist?(backup_file) + end + + def self.username + ActiveRecord::Base.connection.instance_values['config'][:username] + end + + def self.password + ActiveRecord::Base.connection.instance_values['config'][:password] + end + + def self.host + ActiveRecord::Base.connection.instance_values['config'][:host] || '127.0.0.1' + end + + def self.database + ActiveRecord::Base.connection.instance_values['config'][:database] + end + + def self.mysql_arguments + args = " -u#{username} -h#{host}" + args += " -p#{password}" if password.present? # allow for passwordless access on dev systems + args + " #{database}" + end + + def self.rollback + system("mysql #{mysql_arguments} < #{backup_file}", exception: true) + Rake::Task['zammad:db:rebuild'].reenable + Rake::Task['zammad:db:rebuild'].invoke + end + + def self.backup + Rake::Task['zammad:db:reset'].reenable + Rake::Task['zammad:db:reset'].invoke + system("mysqldump #{mysql_arguments} > #{backup_file}", exception: true) + end + + def self.reset + FileUtils.mkdir_p Rails.root.join(basepath) + if backup_exists? + rollback + else + backup + end + end +end diff --git a/lib/tasks/zammad/db/rebuild.rake b/lib/tasks/zammad/db/rebuild.rake new file mode 100644 index 000000000..4d07504de --- /dev/null +++ b/lib/tasks/zammad/db/rebuild.rake @@ -0,0 +1,16 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +namespace :zammad do + + namespace :db do + + desc 'Clears the Cache and reloads the Settings' + task rebuild: :environment do + Package::Migration.linked + ActiveRecord::Base.connection.reconnect! + ActiveRecord::Base.descendants.each(&:reset_column_information) + Cache.clear + Setting.reload + end + end +end diff --git a/lib/tasks/zammad/db/reset.rake b/lib/tasks/zammad/db/reset.rake index f60d0e90e..b88747ddf 100644 --- a/lib/tasks/zammad/db/reset.rake +++ b/lib/tasks/zammad/db/reset.rake @@ -10,7 +10,7 @@ namespace :zammad do # we loop over each dependent task to be able to # execute them and their prerequisites multiple times (in tests) # there is no way in rake to achieve that - %w[db:drop:_unsafe db:create db:migrate db:seed].each do |task| + %w[db:drop:_unsafe db:create db:migrate db:seed zammad:db:rebuild].each do |task| if task == 'db:drop:_unsafe' # ensure all DB connections are closed before dropping the DB @@ -26,12 +26,6 @@ namespace :zammad do ensure $stdout = STDOUT end - - Package::Migration.linked - ActiveRecord::Base.connection.reconnect! - ActiveRecord::Base.descendants.each(&:reset_column_information) - Cache.clear - Setting.reload end end end diff --git a/spec/support/db_strategies.rb b/spec/support/db_strategies.rb index a9cf9d52b..522ff33a4 100644 --- a/spec/support/db_strategies.rb +++ b/spec/support/db_strategies.rb @@ -3,18 +3,17 @@ RSpec.configure do |config| config.around(:each, db_strategy: :reset) do |example| - if ActiveRecord::Base.connection.instance_values['config'][:adapter] != 'postgresql' + if MysqlStrategy.db? self.use_transactional_tests = false end example.run - if ActiveRecord::Base.connection.instance_values['config'][:adapter] == 'postgresql' + if MysqlStrategy.db? + MysqlStrategy.reset + else Models.all.each_key do |model| model.connection.schema_cache.clear! model.reset_column_information end - else - Rake::Task['zammad:db:reset'].reenable - Rake::Task['zammad:db:reset'].invoke end end end