Only read Rack request body if it's rewindable

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

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

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'

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)

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"

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

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)

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|