diff --git a/CHANGELOG.md b/CHANGELOG.md index f8b5bec1..a6f91f35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,15 @@ Changelog ========= +## v6.27.1 (18 June 2024) + +### Fixes + +* 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) ### Enhancements diff --git a/VERSION b/VERSION index 9cd1a39f..72d6521b 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.27.0 +6.27.1 diff --git a/features/fixtures/docker-compose.yml b/features/fixtures/docker-compose.yml index ab99ac02..d54d9dd6 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 1f1fa5fa..2adebf1e 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 b55d644c..7195cd5c 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 3827dae8..bd9e8a65 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 9dee7b3a..29d00efc 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.rb b/lib/bugsnag.rb index 950f1726..40a942e4 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 df7cc5ad..bcf67fc4 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 71ddbb16..a8b5bd3c 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 33d0cd09..5bb08d34 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 bcf64c81..0b4459af 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 b0da7fde..5db31d87 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 2b82b221..3a5d5864 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 af42bb81..b1770fc0 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 7a1c4178..1c030ac0 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 e1b2d22d..8f14de0f 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 f262e63b..021f2ff8 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 9659bce1..4c8aef9b 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 d7613dd1..c875972a 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 6d23668d..7ac173f8 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 387e5df4..b05130eb 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 @@ -17,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"] @@ -106,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? @@ -115,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/lib/bugsnag/report.rb b/lib/bugsnag/report.rb index 92d1b56c..91f8f4d6 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" diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 58c3104b..ae8efb09 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|