From 8389d2b9fdfedea44e2d6e79659e623cad657e2f Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Fri, 30 Mar 2018 18:18:41 -0700 Subject: [PATCH 1/3] fix(rails): Log missing key warning after Rails initialization completes When not initializing using the BUGSNAG_API_KEY environment variable, the first time the Railtie integration is loaded, no API key is available. At the bottom of the configure method, a warning is emitted stating that no API key has been set, though this may not be true after config/initializers/bugsnag.rb runs. This is the behavior being observed in #391, causing concern that bugsnag is not working correctly when in fact the log message is premature. This change allows for Bugsnag.configure to be called without validating the key, allowing the Railtie integration to add an additional check once Rails initialization is complete. Fixes #391 --- lib/bugsnag.rb | 17 ++- lib/bugsnag/integrations/railtie.rb | 2 +- spec/configuration_spec.rb | 26 +++++ .../apps/rails-initializer-config/Gemfile | 4 + .../apps/rails-initializer-config/config.ru | 16 +++ .../config/initializers/bugsnag.rb | 3 + spec/fixtures/apps/rails-no-config/Gemfile | 4 + spec/fixtures/apps/rails-no-config/config.ru | 16 +++ spec/fixtures/apps/scripts/Gemfile | 3 + spec/fixtures/apps/scripts/configure_key.rb | 5 + spec/fixtures/apps/scripts/no_config.rb | 3 + spec/integrations/logger_spec.rb | 106 ++++++++++++++++++ 12 files changed, 198 insertions(+), 7 deletions(-) create mode 100644 spec/fixtures/apps/rails-initializer-config/Gemfile create mode 100644 spec/fixtures/apps/rails-initializer-config/config.ru create mode 100644 spec/fixtures/apps/rails-initializer-config/config/initializers/bugsnag.rb create mode 100644 spec/fixtures/apps/rails-no-config/Gemfile create mode 100644 spec/fixtures/apps/rails-no-config/config.ru create mode 100644 spec/fixtures/apps/scripts/Gemfile create mode 100644 spec/fixtures/apps/scripts/configure_key.rb create mode 100644 spec/fixtures/apps/scripts/no_config.rb create mode 100644 spec/integrations/logger_spec.rb diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index cec483065..6a42567b6 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -37,14 +37,10 @@ class << self # Configure the Bugsnag notifier application-wide settings. # # Yields a configuration object to use to set application settings. - def configure + def configure(validate_api_key=true) yield(configuration) if block_given? - @key_warning = false unless defined?(@key_warning) - if !configuration.valid_api_key? && !@key_warning - configuration.warn("No valid API key has been set, notifications will not be sent") - @key_warning = true - end + check_key_valid if validate_api_key end ## @@ -179,6 +175,15 @@ def load_integration(integration) configuration.debug("Integration #{integration} is not currently supported") end end + + # Check if the API key is valid and warn (once) if it is not + def check_key_valid + @key_warning = false unless defined?(@key_warning) + if !configuration.valid_api_key? && !@key_warning + configuration.warn("No valid API key has been set, notifications will not be sent") + @key_warning = true + end + end end end diff --git a/lib/bugsnag/integrations/railtie.rb b/lib/bugsnag/integrations/railtie.rb index e2c0a4dd9..f9bf8f8c1 100644 --- a/lib/bugsnag/integrations/railtie.rb +++ b/lib/bugsnag/integrations/railtie.rb @@ -36,7 +36,7 @@ class Railtie < Rails::Railtie config.before_initialize do # Configure bugsnag rails defaults - Bugsnag.configure do |config| + Bugsnag.configure(false) do |config| config.logger = ::Rails.logger config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] || ::Rails.env.to_s config.project_root = ::Rails.root.to_s diff --git a/spec/configuration_spec.rb b/spec/configuration_spec.rb index 1313a73c6..32e37a61b 100644 --- a/spec/configuration_spec.rb +++ b/spec/configuration_spec.rb @@ -115,6 +115,32 @@ def debug(name, &block) end end + it "logs a warning when configuring without an API key" do + Bugsnag.configuration.api_key = nil + Bugsnag.instance_variable_set("@key_warning", nil) + ENV['BUGSNAG_API_KEY'] = nil + expect(@logger.logs.size).to eq(0) + Bugsnag.configure + expect(@logger.logs.size).to eq(1) + log = @logger.logs.first + expect(log).to eq({ + :level => "warning", + :name => "[Bugsnag]", + :message => "No valid API key has been set, notifications will not be sent" + }) + end + + it "skips logging a warning when configuring with an API key" do + Bugsnag.configuration.api_key = nil + Bugsnag.instance_variable_set("@key_warning", nil) + ENV['BUGSNAG_API_KEY'] = nil + expect(@logger.logs.size).to eq(0) + Bugsnag.configure do |config| + config.api_key = 'd57a2472bd130ac0ab0f52715bbdc600' + end + expect(@logger.logs.size).to eq(0) + end + it "should log info messages to the set logger" do expect(@logger.logs.size).to eq(0) Bugsnag.configuration.info("Info message") diff --git a/spec/fixtures/apps/rails-initializer-config/Gemfile b/spec/fixtures/apps/rails-initializer-config/Gemfile new file mode 100644 index 000000000..a75e1f79e --- /dev/null +++ b/spec/fixtures/apps/rails-initializer-config/Gemfile @@ -0,0 +1,4 @@ +source 'https://rubygems.org' + +gem 'railties', '4.2.10', require: %w(action_controller rails) +gem 'bugsnag', path: '../../../..' diff --git a/spec/fixtures/apps/rails-initializer-config/config.ru b/spec/fixtures/apps/rails-initializer-config/config.ru new file mode 100644 index 000000000..0dc06cb84 --- /dev/null +++ b/spec/fixtures/apps/rails-initializer-config/config.ru @@ -0,0 +1,16 @@ +Bundler.require + +run InitializerConfigApp ||= Class.new(Rails::Application) { + config.secret_key_base = routes.append { + root to: proc { + [200, {"Content-Type" => "text/plain"}, ["Hello!"]] + } + }.to_s + + config.cache_classes = true + config.eager_load = true + config.logger = Logger.new(STDOUT) + config.log_level = :warn + + initialize! +} diff --git a/spec/fixtures/apps/rails-initializer-config/config/initializers/bugsnag.rb b/spec/fixtures/apps/rails-initializer-config/config/initializers/bugsnag.rb new file mode 100644 index 000000000..427abacb0 --- /dev/null +++ b/spec/fixtures/apps/rails-initializer-config/config/initializers/bugsnag.rb @@ -0,0 +1,3 @@ +Bugsnag.configure do |config| + config.api_key = 'c34a2472bd240ac0ab0f52715bbdc05d' +end diff --git a/spec/fixtures/apps/rails-no-config/Gemfile b/spec/fixtures/apps/rails-no-config/Gemfile new file mode 100644 index 000000000..a75e1f79e --- /dev/null +++ b/spec/fixtures/apps/rails-no-config/Gemfile @@ -0,0 +1,4 @@ +source 'https://rubygems.org' + +gem 'railties', '4.2.10', require: %w(action_controller rails) +gem 'bugsnag', path: '../../../..' diff --git a/spec/fixtures/apps/rails-no-config/config.ru b/spec/fixtures/apps/rails-no-config/config.ru new file mode 100644 index 000000000..d496d4774 --- /dev/null +++ b/spec/fixtures/apps/rails-no-config/config.ru @@ -0,0 +1,16 @@ +Bundler.require + +run NoConfigApp ||= Class.new(Rails::Application) { + config.secret_key_base = routes.append { + root to: proc { + [200, {"Content-Type" => "text/plain"}, ["Hello!"]] + } + }.to_s + + config.cache_classes = true + config.eager_load = true + config.logger = Logger.new(STDOUT) + config.log_level = :warn + + initialize! +} diff --git a/spec/fixtures/apps/scripts/Gemfile b/spec/fixtures/apps/scripts/Gemfile new file mode 100644 index 000000000..a93483fc8 --- /dev/null +++ b/spec/fixtures/apps/scripts/Gemfile @@ -0,0 +1,3 @@ +source 'https://rubygems.org' + +gem 'bugsnag', path: '../../../..' diff --git a/spec/fixtures/apps/scripts/configure_key.rb b/spec/fixtures/apps/scripts/configure_key.rb new file mode 100644 index 000000000..4bef8f98f --- /dev/null +++ b/spec/fixtures/apps/scripts/configure_key.rb @@ -0,0 +1,5 @@ +require 'bugsnag' + +Bugsnag.configure do |config| + config.api_key = 'f25a2472bd230ac0ab0fa2715bbdc654' +end diff --git a/spec/fixtures/apps/scripts/no_config.rb b/spec/fixtures/apps/scripts/no_config.rb new file mode 100644 index 000000000..af078904d --- /dev/null +++ b/spec/fixtures/apps/scripts/no_config.rb @@ -0,0 +1,3 @@ +require 'bugsnag' + +Bugsnag.configure diff --git a/spec/integrations/logger_spec.rb b/spec/integrations/logger_spec.rb new file mode 100644 index 000000000..b64d633b5 --- /dev/null +++ b/spec/integrations/logger_spec.rb @@ -0,0 +1,106 @@ +require 'spec_helper' + +describe 'Configuration.logger' do + + before do + @env = {} + end + + context 'in a Rails app' do + key_warning = '[Bugsnag]: No valid API key has been set, notifications will not be sent' + is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' + incompatible = (RUBY_VERSION < '2.0.0') || is_jruby + + before do + skip "Incompatible with Ruby <2.0 and JRuby" if incompatible + @env['RACK_ENV'] = 'production' + end + + def run_app(name) + out_reader, out_writer = IO.pipe + Dir.chdir(File.join(File.dirname(__FILE__), "../fixtures/apps/#{name}")) do + Bundler.with_clean_env do + pid = Process.spawn('bundle install', + out: out_writer.fileno, + err: out_writer.fileno) + Process.waitpid(pid, 0) + pid = Process.spawn(@env, 'bundle exec rackup config.ru', + out: out_writer.fileno, + err: out_writer.fileno) + sleep(2) + Process.kill(1, pid) + end + end + out_writer.close + output = "" + output << out_reader.gets until out_reader.eof? + output + end + context 'sets an API key using the BUGSNAG_API_KEY env var' do + it 'does not log a warning' do + skip "Incompatible with Ruby <2.0 and JRuby" if incompatible + @env['BUGSNAG_API_KEY'] = 'c34a2472bd240ac0ab0f52715bbdc05d' + output = run_app('rails-no-config') + expect(output).not_to include(key_warning) + end + end + + context 'sets an API key using the bugsnag initializer' do + it 'does not log a warning' do + skip "Incompatible with Ruby <2.0 and JRuby" if incompatible + output = run_app('rails-initializer-config') + expect(output).not_to include(key_warning) + end + end + + context 'skips setting an API key' do + it 'logs a warning' do + skip "Incompatible with Ruby <2.0 and JRuby" if incompatible + output = run_app('rails-no-config') + expect(output).to include(key_warning) + end + end + end + + context 'in a script' do + key_warning = /\[Bugsnag\] .* No valid API key has been set, notifications will not be sent/ + + def run_app(name) + out_reader, out_writer = IO.pipe + Dir.chdir(File.join(File.dirname(__FILE__), "../fixtures/apps/scripts")) do + Bundler.with_clean_env do + pid = Process.spawn(@env, "bundle exec ruby #{name}.rb", + out: out_writer.fileno, + err: out_writer.fileno) + Process.waitpid(pid, 0) + end + end + out_writer.close + output = "" + output << out_reader.gets until out_reader.eof? + output + end + + context 'sets an API key using the BUGSNAG_API_KEY env var' do + it 'does not log a warning' do + @env['BUGSNAG_API_KEY'] = 'c34a2472bd240ac0ab0f52715bbdc05d' + output = run_app('no_config') + expect(output).not_to match(key_warning) + end + end + + context 'sets an API key using Bugsnag.configure' do + it 'does not log a warning' do + output = run_app('configure_key') + expect(output).not_to match(key_warning) + end + end + + context 'skips setting an API key' do + it 'logs a warning' do + output = run_app('no_config') + expect(output).to match(key_warning) + end + end + end +end From 0e09ae0f532f6c02a6404a9b64848c47892f6067 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Fri, 6 Apr 2018 15:11:47 -0700 Subject: [PATCH 2/3] tests: Add additional tests for configuring with invalid keys --- spec/configuration_spec.rb | 78 +++++++++++++------ .../rails-invalid-initializer-config/Gemfile | 4 + .../config.ru | 16 ++++ .../config/initializers/bugsnag.rb | 3 + .../apps/scripts/configure_invalid_key.rb | 5 ++ spec/integrations/logger_spec.rb | 32 ++++++++ 6 files changed, 116 insertions(+), 22 deletions(-) create mode 100644 spec/fixtures/apps/rails-invalid-initializer-config/Gemfile create mode 100644 spec/fixtures/apps/rails-invalid-initializer-config/config.ru create mode 100644 spec/fixtures/apps/rails-invalid-initializer-config/config/initializers/bugsnag.rb create mode 100644 spec/fixtures/apps/scripts/configure_invalid_key.rb diff --git a/spec/configuration_spec.rb b/spec/configuration_spec.rb index 32e37a61b..4f1abb6c1 100644 --- a/spec/configuration_spec.rb +++ b/spec/configuration_spec.rb @@ -115,30 +115,64 @@ def debug(name, &block) end end - it "logs a warning when configuring without an API key" do - Bugsnag.configuration.api_key = nil - Bugsnag.instance_variable_set("@key_warning", nil) - ENV['BUGSNAG_API_KEY'] = nil - expect(@logger.logs.size).to eq(0) - Bugsnag.configure - expect(@logger.logs.size).to eq(1) - log = @logger.logs.first - expect(log).to eq({ - :level => "warning", - :name => "[Bugsnag]", - :message => "No valid API key has been set, notifications will not be sent" - }) - end + context "using configure" do + before do + Bugsnag.configuration.api_key = nil + Bugsnag.instance_variable_set("@key_warning", nil) + ENV['BUGSNAG_API_KEY'] = nil + expect(@logger.logs.size).to eq(0) + end - it "skips logging a warning when configuring with an API key" do - Bugsnag.configuration.api_key = nil - Bugsnag.instance_variable_set("@key_warning", nil) - ENV['BUGSNAG_API_KEY'] = nil - expect(@logger.logs.size).to eq(0) - Bugsnag.configure do |config| - config.api_key = 'd57a2472bd130ac0ab0f52715bbdc600' + context "API key is not specified" do + it "skips logging a warning if validate_api_key is false" do + Bugsnag.configure(false) + expect(@logger.logs.size).to eq(0) + end + + it "logs a warning by default" do + Bugsnag.configure + expect(@logger.logs.size).to eq(1) + log = @logger.logs.first + expect(log).to eq({ + :level => "warning", + :name => "[Bugsnag]", + :message => "No valid API key has been set, notifications will not be sent" + }) + end + + it "logs a warning if validate_api_key is true" do + Bugsnag.configure(true) + expect(@logger.logs.size).to eq(1) + log = @logger.logs.first + expect(log).to eq({ + :level => "warning", + :name => "[Bugsnag]", + :message => "No valid API key has been set, notifications will not be sent" + }) + end + end + + context "API key is set" do + it "skips logging a warning when configuring with an API key" do + Bugsnag.configure do |config| + config.api_key = 'd57a2472bd130ac0ab0f52715bbdc600' + end + expect(@logger.logs.size).to eq(0) + end + + it "logs a warning if the configured API key is invalid" do + Bugsnag.configure do |config| + config.api_key = 'WARNING: not a real key' + end + expect(@logger.logs.size).to eq(1) + log = @logger.logs.first + expect(log).to eq({ + :level => "warning", + :name => "[Bugsnag]", + :message => "No valid API key has been set, notifications will not be sent" + }) + end end - expect(@logger.logs.size).to eq(0) end it "should log info messages to the set logger" do diff --git a/spec/fixtures/apps/rails-invalid-initializer-config/Gemfile b/spec/fixtures/apps/rails-invalid-initializer-config/Gemfile new file mode 100644 index 000000000..a75e1f79e --- /dev/null +++ b/spec/fixtures/apps/rails-invalid-initializer-config/Gemfile @@ -0,0 +1,4 @@ +source 'https://rubygems.org' + +gem 'railties', '4.2.10', require: %w(action_controller rails) +gem 'bugsnag', path: '../../../..' diff --git a/spec/fixtures/apps/rails-invalid-initializer-config/config.ru b/spec/fixtures/apps/rails-invalid-initializer-config/config.ru new file mode 100644 index 000000000..0dc06cb84 --- /dev/null +++ b/spec/fixtures/apps/rails-invalid-initializer-config/config.ru @@ -0,0 +1,16 @@ +Bundler.require + +run InitializerConfigApp ||= Class.new(Rails::Application) { + config.secret_key_base = routes.append { + root to: proc { + [200, {"Content-Type" => "text/plain"}, ["Hello!"]] + } + }.to_s + + config.cache_classes = true + config.eager_load = true + config.logger = Logger.new(STDOUT) + config.log_level = :warn + + initialize! +} diff --git a/spec/fixtures/apps/rails-invalid-initializer-config/config/initializers/bugsnag.rb b/spec/fixtures/apps/rails-invalid-initializer-config/config/initializers/bugsnag.rb new file mode 100644 index 000000000..9ff5342c0 --- /dev/null +++ b/spec/fixtures/apps/rails-invalid-initializer-config/config/initializers/bugsnag.rb @@ -0,0 +1,3 @@ +Bugsnag.configure do |config| + config.api_key = '1' +end diff --git a/spec/fixtures/apps/scripts/configure_invalid_key.rb b/spec/fixtures/apps/scripts/configure_invalid_key.rb new file mode 100644 index 000000000..f376b4cd4 --- /dev/null +++ b/spec/fixtures/apps/scripts/configure_invalid_key.rb @@ -0,0 +1,5 @@ +require 'bugsnag' + +Bugsnag.configure do |config| + config.api_key = 'no' +end diff --git a/spec/integrations/logger_spec.rb b/spec/integrations/logger_spec.rb index b64d633b5..dc5cd19b9 100644 --- a/spec/integrations/logger_spec.rb +++ b/spec/integrations/logger_spec.rb @@ -60,6 +60,23 @@ def run_app(name) expect(output).to include(key_warning) end end + + context 'sets an invalid API key using the BUGSNAG_API_KEY env var' do + it 'logs a warning' do + skip "Incompatible with Ruby <2.0 and JRuby" if incompatible + output = run_app('rails-invalid-initializer-config') + expect(output).to include(key_warning) + end + end + + context 'sets an invalid API key using the BUGSNAG_API_KEY env var' do + it 'logs a warning' do + skip "Incompatible with Ruby <2.0 and JRuby" if incompatible + @env['BUGSNAG_API_KEY'] = 'not a real key' + output = run_app('rails-no-config') + expect(output).to include(key_warning) + end + end end context 'in a script' do @@ -96,6 +113,21 @@ def run_app(name) end end + context 'sets an invalid API key using Bugsnag.configure' do + it 'logs a warning' do + output = run_app('configure_invalid_key') + expect(output).to match(key_warning) + end + end + + context 'sets an invalid API key using the BUGSNAG_API_KEY env var' do + it 'logs a warning' do + @env['BUGSNAG_API_KEY'] = 'bad key bad key whatcha gonna do' + output = run_app('no_config') + expect(output).to match(key_warning) + end + end + context 'skips setting an API key' do it 'logs a warning' do output = run_app('no_config') From 50c052552e6bd4511fd19853ea7175cb72c230e1 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Fri, 6 Apr 2018 15:12:47 -0700 Subject: [PATCH 3/3] docs(rails): Indicate why API key validation is skipped --- lib/bugsnag/integrations/railtie.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/bugsnag/integrations/railtie.rb b/lib/bugsnag/integrations/railtie.rb index f9bf8f8c1..be1a6b64f 100644 --- a/lib/bugsnag/integrations/railtie.rb +++ b/lib/bugsnag/integrations/railtie.rb @@ -36,6 +36,8 @@ class Railtie < Rails::Railtie config.before_initialize do # Configure bugsnag rails defaults + # Skipping API key validation as the key may be set later in an + # initializer. If not, the key will be validated in after_initialize. Bugsnag.configure(false) do |config| config.logger = ::Rails.logger config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] || ::Rails.env.to_s