From 946521e06c79a9c8e9ca37bd7a4e4633ddce5458 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Mon, 10 Jun 2024 11:19:51 +0100 Subject: [PATCH 1/4] Organise requires to fix circular require warning --- lib/bugsnag.rb | 17 ++++------- .../on_breadcrumb_callback_list.rb | 2 -- lib/bugsnag/breadcrumbs/validator.rb | 2 -- lib/bugsnag/cleaner.rb | 2 -- lib/bugsnag/configuration.rb | 30 ++++++++++++------- lib/bugsnag/delivery/synchronous.rb | 1 - lib/bugsnag/delivery/thread_queue.rb | 2 -- lib/bugsnag/event.rb | 2 -- lib/bugsnag/helpers.rb | 5 ---- lib/bugsnag/integrations/mongo.rb | 1 - lib/bugsnag/integrations/rails/active_job.rb | 2 -- .../integrations/rails/rails_breadcrumbs.rb | 2 -- lib/bugsnag/integrations/railtie.rb | 4 --- lib/bugsnag/integrations/rake.rb | 5 +++- lib/bugsnag/middleware/rack_request.rb | 2 -- lib/bugsnag/report.rb | 1 - 16 files changed, 28 insertions(+), 52 deletions(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index 950f17267..40a942e46 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -1,5 +1,10 @@ require "rubygems" require "thread" +require "set" +require "json" +require "uri" +require "socket" +require "logger" require "bugsnag/version" require "bugsnag/utility/feature_data_store" @@ -21,21 +26,9 @@ # as it doesn't auto-configure when loaded require "bugsnag/integrations/rack" -require "bugsnag/middleware/rack_request" -require "bugsnag/middleware/warden_user" -require "bugsnag/middleware/clearance_user" -require "bugsnag/middleware/callbacks" -require "bugsnag/middleware/rails3_request" -require "bugsnag/middleware/sidekiq" -require "bugsnag/middleware/mailman" -require "bugsnag/middleware/rake" -require "bugsnag/middleware/classify_error" -require "bugsnag/middleware/delayed_job" - require "bugsnag/breadcrumb_type" require "bugsnag/breadcrumbs/validator" require "bugsnag/breadcrumbs/breadcrumb" -require "bugsnag/breadcrumbs/breadcrumbs" require "bugsnag/utility/duplicator" require "bugsnag/utility/metadata_delegate" diff --git a/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb b/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb index df7cc5add..bcf67fc4a 100644 --- a/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb +++ b/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb @@ -1,5 +1,3 @@ -require "set" - module Bugsnag::Breadcrumbs class OnBreadcrumbCallbackList def initialize(configuration) diff --git a/lib/bugsnag/breadcrumbs/validator.rb b/lib/bugsnag/breadcrumbs/validator.rb index 71ddbb16a..a8b5bd3c3 100644 --- a/lib/bugsnag/breadcrumbs/validator.rb +++ b/lib/bugsnag/breadcrumbs/validator.rb @@ -1,5 +1,3 @@ -require 'bugsnag/breadcrumbs/breadcrumbs' - module Bugsnag::Breadcrumbs ## # Validates a given breadcrumb before it is stored diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index 33d0cd097..5bb08d34b 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -1,5 +1,3 @@ -require 'uri' - module Bugsnag # @api private class Cleaner diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index bcf64c813..0b4459af2 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -1,20 +1,28 @@ -require "set" -require "socket" -require "logger" -require "bugsnag/middleware_stack" +require "bugsnag/breadcrumbs/on_breadcrumb_callback_list" + +require "bugsnag/endpoint_configuration" +require "bugsnag/endpoint_validator" + +require "bugsnag/middleware/breadcrumbs" require "bugsnag/middleware/callbacks" +require "bugsnag/middleware/classify_error" +require "bugsnag/middleware/clearance_user" +require "bugsnag/middleware/delayed_job" require "bugsnag/middleware/discard_error_class" require "bugsnag/middleware/exception_meta_data" require "bugsnag/middleware/ignore_error_class" -require "bugsnag/middleware/suggestion_data" -require "bugsnag/middleware/classify_error" +require "bugsnag/middleware/mailman" +require "bugsnag/middleware/rack_request" +require "bugsnag/middleware/rails3_request" +require "bugsnag/middleware/rake" require "bugsnag/middleware/session_data" -require "bugsnag/middleware/breadcrumbs" +require "bugsnag/middleware/sidekiq" +require "bugsnag/middleware/suggestion_data" +require "bugsnag/middleware/warden_user" + +require "bugsnag/middleware_stack" + require "bugsnag/utility/circular_buffer" -require "bugsnag/breadcrumbs/breadcrumbs" -require "bugsnag/breadcrumbs/on_breadcrumb_callback_list" -require "bugsnag/endpoint_configuration" -require "bugsnag/endpoint_validator" module Bugsnag class Configuration diff --git a/lib/bugsnag/delivery/synchronous.rb b/lib/bugsnag/delivery/synchronous.rb index b0da7fde5..5db31d87d 100644 --- a/lib/bugsnag/delivery/synchronous.rb +++ b/lib/bugsnag/delivery/synchronous.rb @@ -1,5 +1,4 @@ require "net/https" -require "uri" module Bugsnag module Delivery diff --git a/lib/bugsnag/delivery/thread_queue.rb b/lib/bugsnag/delivery/thread_queue.rb index 2b82b2212..3a5d5864b 100644 --- a/lib/bugsnag/delivery/thread_queue.rb +++ b/lib/bugsnag/delivery/thread_queue.rb @@ -1,5 +1,3 @@ -require "thread" - module Bugsnag module Delivery class ThreadQueue < Synchronous diff --git a/lib/bugsnag/event.rb b/lib/bugsnag/event.rb index af42bb814..b1770fc03 100644 --- a/lib/bugsnag/event.rb +++ b/lib/bugsnag/event.rb @@ -1,5 +1,3 @@ -require "bugsnag/report" - module Bugsnag # For now Event is just an alias of Report. This points to the same object so # any changes to Report will also affect Event diff --git a/lib/bugsnag/helpers.rb b/lib/bugsnag/helpers.rb index 7a1c41786..1c030ac0c 100644 --- a/lib/bugsnag/helpers.rb +++ b/lib/bugsnag/helpers.rb @@ -1,8 +1,3 @@ -require 'uri' -require 'set' -require 'json' - - module Bugsnag module Helpers # rubocop:todo Metrics/ModuleLength MAX_STRING_LENGTH = 3072 diff --git a/lib/bugsnag/integrations/mongo.rb b/lib/bugsnag/integrations/mongo.rb index e1b2d22d0..8f14de0fd 100644 --- a/lib/bugsnag/integrations/mongo.rb +++ b/lib/bugsnag/integrations/mongo.rb @@ -1,5 +1,4 @@ require 'mongo' -require 'bugsnag/breadcrumbs/breadcrumbs' module Bugsnag ## diff --git a/lib/bugsnag/integrations/rails/active_job.rb b/lib/bugsnag/integrations/rails/active_job.rb index f262e63b0..021f2ff88 100644 --- a/lib/bugsnag/integrations/rails/active_job.rb +++ b/lib/bugsnag/integrations/rails/active_job.rb @@ -1,5 +1,3 @@ -require 'set' - module Bugsnag::Rails module ActiveJob SEVERITY = 'error' diff --git a/lib/bugsnag/integrations/rails/rails_breadcrumbs.rb b/lib/bugsnag/integrations/rails/rails_breadcrumbs.rb index 9659bce19..4c8aef9b5 100644 --- a/lib/bugsnag/integrations/rails/rails_breadcrumbs.rb +++ b/lib/bugsnag/integrations/rails/rails_breadcrumbs.rb @@ -1,5 +1,3 @@ -require "bugsnag/breadcrumbs/breadcrumbs" - module Bugsnag::Rails DEFAULT_RAILS_BREADCRUMBS = [ { diff --git a/lib/bugsnag/integrations/railtie.rb b/lib/bugsnag/integrations/railtie.rb index d7613dd17..c875972a8 100644 --- a/lib/bugsnag/integrations/railtie.rb +++ b/lib/bugsnag/integrations/railtie.rb @@ -1,10 +1,6 @@ # Rails 3.x hooks -require "json" require "rails" -require "bugsnag" -require "bugsnag/middleware/rails3_request" -require "bugsnag/middleware/rack_request" require "bugsnag/integrations/rails/rails_breadcrumbs" module Bugsnag diff --git a/lib/bugsnag/integrations/rake.rb b/lib/bugsnag/integrations/rake.rb index 6d23668d9..7ac173f8b 100644 --- a/lib/bugsnag/integrations/rake.rb +++ b/lib/bugsnag/integrations/rake.rb @@ -1,4 +1,7 @@ -require 'bugsnag' +# this file can either be required manually by a user, in which case 'bugsnag' +# needs to be required, or it can be required automatically in the railtie, +# in which case 'bugsnag' has already been required +require 'bugsnag' unless defined?(Bugsnag) Rake::TaskManager.record_task_metadata = true diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 387e5df47..94cb420b2 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -1,5 +1,3 @@ -require "json" - module Bugsnag::Middleware ## # Extracts and attaches rack data to an error report diff --git a/lib/bugsnag/report.rb b/lib/bugsnag/report.rb index 92d1b56cb..91f8f4d61 100644 --- a/lib/bugsnag/report.rb +++ b/lib/bugsnag/report.rb @@ -1,4 +1,3 @@ -require "json" require "pathname" require "bugsnag/error" require "bugsnag/stacktrace" From fa5d160f1e5cd70e6ffea9b1654fe33563acd616 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Mon, 17 Jun 2024 11:46:24 +0100 Subject: [PATCH 2/4] Only read Rack request body if it's rewindable --- CHANGELOG.md | 7 ++++ features/fixtures/docker-compose.yml | 1 + features/fixtures/rack/app/Gemfile | 4 +- features/fixtures/rack/app/app.rb | 7 +++- features/rack.feature | 58 ++++++++++++++++++++++++++ features/support/env.rb | 8 ++++ lib/bugsnag/middleware/rack_request.rb | 50 +++++++++++++++------- spec/integrations/rack_spec.rb | 24 +++++++++++ 8 files changed, 140 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8b5bec13..7341a60fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ Changelog ========= +## TBD + +### Fixes + +* Only read Rack request body if it's rewindable + | [#829](https://github.com/bugsnag/bugsnag-ruby/pull/829) + ## v6.27.0 (23 May 2024) ### Enhancements diff --git a/features/fixtures/docker-compose.yml b/features/fixtures/docker-compose.yml index ab99ac022..d54d9dd6c 100644 --- a/features/fixtures/docker-compose.yml +++ b/features/fixtures/docker-compose.yml @@ -88,6 +88,7 @@ services: - BUGSNAG_API_KEY - BUGSNAG_ENDPOINT - BUGSNAG_METADATA_FILTERS + - BUGSNAG_RACK_NO_REWIND restart: "no" ports: - target: 3000 diff --git a/features/fixtures/rack/app/Gemfile b/features/fixtures/rack/app/Gemfile index 1f1fa5fa3..2adebf1ec 100644 --- a/features/fixtures/rack/app/Gemfile +++ b/features/fixtures/rack/app/Gemfile @@ -6,6 +6,4 @@ gem 'webrick' if Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('3.0.0') # Some functionality provided by Rack was moved to the 'rackup' gem in Rack v3 # Specifically the test app uses Rack::Server, which is now Rackup::Server -if ENV['RACK_VERSION'] == '3' - gem 'rackup', '~> 0.2.3' -end +gem 'rackup' if ENV['RACK_VERSION'] >= '3' diff --git a/features/fixtures/rack/app/app.rb b/features/fixtures/rack/app/app.rb index b55d644cf..7195cd5cd 100644 --- a/features/fixtures/rack/app/app.rb +++ b/features/fixtures/rack/app/app.rb @@ -71,12 +71,17 @@ def call(env) end end +app = Bugsnag::Rack.new(BugsnagTests.new) + Server = if defined?(Rack::Server) Rack::Server else require 'rackup' + + app = Rack::RewindableInput::Middleware.new(app) unless ENV["BUGSNAG_RACK_NO_REWIND"] == "true" + Rackup::Server end -Server.start(app: Bugsnag::Rack.new(BugsnagTests.new), Host: '0.0.0.0', Port: 3000) +Server.start(app: app, Host: '0.0.0.0', Port: 3000) diff --git a/features/rack.feature b/features/rack.feature index 3827dae8c..bd9e8a654 100644 --- a/features/rack.feature +++ b/features/rack.feature @@ -64,6 +64,9 @@ Scenario: A POST request with form data sends a report with the parsed request b And the event "metaData.request.httpVersion" matches "^HTTP/\d\.\d$" And the event "metaData.request.params.a" equals "123" And the event "metaData.request.params.b" equals "456" + And the event "metaData.request.params.name" equals "baba" + And the event "metaData.request.params.favourite_letter" equals "z" + And the event "metaData.request.params.password" equals "[FILTERED]" And the event "metaData.request.referer" is null And the event "metaData.request.url" ends with "/unhandled?a=123&b=456" @@ -86,6 +89,9 @@ Scenario: A POST request with JSON sends a report with the parsed request body a And the event "metaData.request.httpVersion" matches "^HTTP/\d\.\d$" And the event "metaData.request.params.a" equals "123" And the event "metaData.request.params.b" equals "456" + And the event "metaData.request.params.name" is null + And the event "metaData.request.params.favourite_letter" is null + And the event "metaData.request.params.password" is null And the event "metaData.request.referer" is null And the event "metaData.request.url" ends with "/unhandled?a=123&b=456" @@ -172,3 +178,55 @@ Scenario: clearing feature flags for an unhandled error And I wait to receive an error Then the error is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier" notifier And the event has no feature flags + +@not-rack-1 +@not-rack-2 +Scenario: An unrewindable POST request with form data does not attach request body + Given I set environment variable "BUGSNAG_RACK_NO_REWIND" to "true" + And I start the rack service + When I send a POST request to "/unhandled?a=123&b=456" in the rack app with the following form data: + | name | baba | + | favourite_letter | z | + | password | password1 | + And I wait to receive an error + Then the error is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier" notifier + And the event "metaData.request.body" is null + And the event "metaData.request.clientIp" is not null + And the event "metaData.request.cookies" is null + And the event "metaData.request.headers.Host" is not null + And the event "metaData.request.headers.User-Agent" is not null + And the event "metaData.request.httpMethod" equals "POST" + And the event "metaData.request.httpVersion" matches "^HTTP/\d\.\d$" + And the event "metaData.request.params.a" equals "123" + And the event "metaData.request.params.b" equals "456" + And the event "metaData.request.params.name" is null + And the event "metaData.request.params.favourite_letter" is null + And the event "metaData.request.params.password" is null + And the event "metaData.request.referer" is null + And the event "metaData.request.url" ends with "/unhandled?a=123&b=456" + +@not-rack-1 +@not-rack-2 +Scenario: An unrewindable POST request with JSON does not attach request body + Given I set environment variable "BUGSNAG_RACK_NO_REWIND" to "true" + And I start the rack service + When I send a POST request to "/unhandled?a=123&b=456" in the rack app with the following JSON: + | name | baba | + | favourite_letter | z | + | password | password1 | + And I wait to receive an error + Then the error is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier" notifier + And the event "metaData.request.body" is null + And the event "metaData.request.clientIp" is not null + And the event "metaData.request.cookies" is null + And the event "metaData.request.headers.Host" is not null + And the event "metaData.request.headers.User-Agent" is not null + And the event "metaData.request.httpMethod" equals "POST" + And the event "metaData.request.httpVersion" matches "^HTTP/\d\.\d$" + And the event "metaData.request.params.a" equals "123" + And the event "metaData.request.params.b" equals "456" + And the event "metaData.request.params.name" is null + And the event "metaData.request.params.favourite_letter" is null + And the event "metaData.request.params.password" is null + And the event "metaData.request.referer" is null + And the event "metaData.request.url" ends with "/unhandled?a=123&b=456" diff --git a/features/support/env.rb b/features/support/env.rb index 9dee7b3a1..29d00efc4 100644 --- a/features/support/env.rb +++ b/features/support/env.rb @@ -63,3 +63,11 @@ def current_ip Maze::Runner.environment["BUGSNAG_ENDPOINT"] = "http://#{host}:#{Maze.config.port}/notify" Maze::Runner.environment["BUGSNAG_SESSION_ENDPOINT"] = "http://#{host}:#{Maze.config.port}/sessions" end + +Before("@not-rack-1") do + skip_this_scenario if ENV["RACK_VERSION"] == "1" +end + +Before("@not-rack-2") do + skip_this_scenario if ENV["RACK_VERSION"] == "2" +end diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 94cb420b2..b05130eba 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -15,7 +15,15 @@ def call(report) request = ::Rack::Request.new(env) - params = request.params rescue {} + params = + # if the request body isn't rewindable then we can't read request.POST + # which is used internally by request.params + if request.body.respond_to?(:rewind) + request.params rescue {} + else + request.GET rescue {} + end + client_ip = request.ip.to_s rescue SPOOF session = env["rack.session"] @@ -104,7 +112,11 @@ def format_headers(env, referer) end def add_request_body(report, request, env) - body = parsed_request_body(request, env) + begin + body = parsed_request_body(request, env) + rescue StandardError + return nil + end # this request may not have a body return unless body.is_a?(Hash) && !body.empty? @@ -113,26 +125,34 @@ def add_request_body(report, request, env) end def parsed_request_body(request, env) - return request.POST rescue nil if request.form_data? + # if the request is not rewindable then either: + # - it's been read already and so is impossible to read + # - it hasn't been read yet and us reading it will prevent the user from + # reading it themselves + # in either case we should avoid attempting to + return nil unless request.body.respond_to?(:rewind) + + if request.form_data? + begin + return request.POST + ensure + request.body.rewind + end + end content_type = env["CONTENT_TYPE"] return nil if content_type.nil? + return nil unless content_type.include?('/json') || content_type.include?('+json') - if content_type.include?('/json') || content_type.include?('+json') - begin - body = request.body + begin + body = request.body - return JSON.parse(body.read) - rescue StandardError - return nil - ensure - # the body must be rewound so other things can read it after we do - body.rewind - end + JSON.parse(body.read) + ensure + # the body must be rewound so other things can read it after we do + body.rewind end - - nil end def add_cookies(report, request) diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 58c3104b5..ae8efb092 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -104,7 +104,10 @@ class Request } rack_request = double + rack_request_body = double + allow(rack_request).to receive_messages( + body: rack_request_body, params: { param: 'test', param2: 'test2' }, ip: "rack_ip", request_method: "TEST", @@ -119,6 +122,9 @@ class Request cookies: { session_id: 12345 } ) + allow(rack_request_body).to receive(:respond_to?).with(:rewind).and_return(true) + allow(rack_request_body).to receive(:rewind) + expect(::Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) Bugsnag.configure do |config| @@ -160,7 +166,10 @@ class Request } rack_request = double + rack_request_body = double + allow(rack_request).to receive_messages( + body: rack_request_body, params: { param: 'test', param2: 'test2' }, ip: "rack_ip", request_method: "TEST", @@ -175,6 +184,9 @@ class Request cookies: { session_id: 12345 } ) + allow(rack_request_body).to receive(:respond_to?).with(:rewind).and_return(true) + allow(rack_request_body).to receive(:rewind) + expect(::Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) Bugsnag.configure do |config| @@ -218,7 +230,10 @@ class Request } rack_request = double + rack_request_body = double + allow(rack_request).to receive_messages( + body: rack_request_body, params: { param: 'test', param2: 'test2' }, ip: "rack_ip", request_method: "TEST", @@ -233,6 +248,9 @@ class Request cookies: { session_id: 12345 } ) + allow(rack_request_body).to receive(:respond_to?).with(:rewind).and_return(true) + allow(rack_request_body).to receive(:rewind) + expect(Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) Bugsnag.configure do |config| @@ -274,7 +292,10 @@ class Request } rack_request = double + rack_request_body = double + allow(rack_request).to receive_messages( + body: rack_request_body, params: { param: 'test', param2: 'test2' }, ip: "rack_ip", request_method: "TEST", @@ -290,6 +311,9 @@ class Request cookies: {} ) + allow(rack_request_body).to receive(:respond_to?).with(:rewind).and_return(true) + allow(rack_request_body).to receive(:rewind) + expect(Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) Bugsnag.configure do |config| From db4e9050511e082de29188e907df5fca57c19d7f Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 18 Jun 2024 09:45:41 +0100 Subject: [PATCH 3/4] Add #828 to changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7341a60fb..81311f3ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ Changelog * Only read Rack request body if it's rewindable | [#829](https://github.com/bugsnag/bugsnag-ruby/pull/829) +* Fix circular require warning + | [#828](https://github.com/bugsnag/bugsnag-ruby/pull/828) ## v6.27.0 (23 May 2024) From b19ea1afe2d2d5451516a480ed0d7148bd17042a Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 18 Jun 2024 09:44:59 +0100 Subject: [PATCH 4/4] Bump version --- CHANGELOG.md | 2 +- VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81311f3ab..a6f91f35d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ Changelog ========= -## TBD +## v6.27.1 (18 June 2024) ### Fixes diff --git a/VERSION b/VERSION index 9cd1a39f6..72d6521b9 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.27.0 +6.27.1