Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
```
- Add `config.profiles_sample_interval` to control sampling frequency ([#2745](https://github.com/getsentry/sentry-ruby/pull/2745))
- Both `stackprof` and `vernier` now get sampled at a default frequency of 101 Hz.
- Request body reading checks for `:rewind` to match Rack 3 behavior. ([#2754](https://github.com/getsentry/sentry-ruby/pull/2754))

### Internal

Expand Down
2 changes: 2 additions & 0 deletions sentry-ruby/lib/sentry/interfaces/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ def initialize(env:, send_default_pii:, rack_env_whitelist:)
private

def read_data_from(request)
return "Skipped non-rewindable request body" unless request.body.respond_to?(:rewind)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Form Data Request Handling Bug

The early request.body.respond_to?(:rewind) check incorrectly skips form data requests that don't require rewinding. It also changes the return value from nil to a "Skipped non-rewindable request body" string when request.body is nil, affecting RequestInterface.data for empty bodies.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they do require rewinding, it's implicit currently

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: I feel a symbol might be cleaner than returning a string, or even a constant with the symbol as value 🤷 and is this something that's worth logging with sdk_logger at debug level, or is that too much?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will surface in the product it's part of the payload so they can add the rewindable middleware if they want to fix it


if request.form_data?
request.POST
elsif request.body # JSON requests, etc
Expand Down
15 changes: 15 additions & 0 deletions sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,21 @@ def to_s
expect(subject.data).to eq("catch me")
end

it "does not try to read non rewindable body" do
env.merge!(::Rack::RACK_INPUT => double)

expect(subject.data).to eq("Skipped non-rewindable request body")
end

it "reads rewindable body" do
dbl = double
allow(dbl).to receive(:rewind)
allow(dbl).to receive(:read).and_return("stuff")
env.merge!(::Rack::RACK_INPUT => dbl)

expect(subject.data).to eq("stuff")
end

it "stores Authorization header" do
env.merge!("HTTP_AUTHORIZATION" => "Basic YWxhZGRpbjpvcGVuc2VzYW1l")

Expand Down
86 changes: 86 additions & 0 deletions sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,92 @@ def inspect
expect(Sentry.get_current_scope.tags).to eq(tag_1: "don't change me")
end
end

context "with send_default_pii" do
before do
perform_basic_setup do |config|
config.send_default_pii = true
end
end

context "with form data" do
let(:additional_headers) do
{ "REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("foo=bar") }
end

it "captures the exception with request form data" do
app = ->(_e) { raise exception }
stack = Sentry::Rack::CaptureExceptions.new(app)

expect { stack.call(env) }.to raise_error(ZeroDivisionError)

event = last_sentry_event.to_h
expect(event.dig(:request, :url)).to eq("http://example.org/test")
expect(event.dig(:request, :data)).to eq({ "foo" => "bar" })
end

it "allows later middlewares to read body" do
app = ->(_e) { raise exception }
stack = Sentry::Rack::CaptureExceptions.new(app)

expect { stack.call(env) }.to raise_error(ZeroDivisionError)
expect { ::Rack::Request.new(env).body.read }.not_to raise_error
end
end

context "with rewindable non form data" do
let(:additional_headers) do
{ "REQUEST_METHOD" => "POST", "CONTENT_TYPE" => "application/text", ::Rack::RACK_INPUT => StringIO.new("stuff") }
end

it "captures the exception with request form data" do
app = ->(_e) { raise exception }
stack = Sentry::Rack::CaptureExceptions.new(app)

expect { stack.call(env) }.to raise_error(ZeroDivisionError)

event = last_sentry_event.to_h
expect(event.dig(:request, :url)).to eq("http://example.org/test")
expect(event.dig(:request, :data)).to eq("stuff")
end

it "allows later middlewares to read body" do
app = ->(_e) { raise exception }
stack = Sentry::Rack::CaptureExceptions.new(app)

expect { stack.call(env) }.to raise_error(ZeroDivisionError)
expect { ::Rack::Request.new(env).body.read }.not_to raise_error
end
end

context "with non rewindable non form data" do
let(:dbl) { double }
let(:additional_headers) do
{ "REQUEST_METHOD" => "POST", "CONTENT_TYPE" => "application/text", ::Rack::RACK_INPUT => dbl }
end

it "does not try to read non rewindable body" do
app = ->(_e) { raise exception }
stack = Sentry::Rack::CaptureExceptions.new(app)

expect { stack.call(env) }.to raise_error(ZeroDivisionError)

event = last_sentry_event.to_h
expect(event.dig(:request, :url)).to eq("http://example.org/test")
expect(event.dig(:request, :data)).to eq("Skipped non-rewindable request body")
end

it "allows later middlewares to read body" do
allow(dbl).to receive(:read)

app = ->(_e) { raise exception }
stack = Sentry::Rack::CaptureExceptions.new(app)

expect { stack.call(env) }.to raise_error(ZeroDivisionError)
expect { ::Rack::Request.new(env).body.read }.not_to raise_error
end
end
end
end

describe "performance monitoring" do
Expand Down
Loading