diff --git a/CHANGELOG.md b/CHANGELOG.md index f8b5bec1..7341a60f 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 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/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 94cb420b..b05130eb 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 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|