diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f7284006..e74760fb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Remove `config.async` [#1894](https://github.com/getsentry/sentry-ruby/pull/1894) - Migrate from to_hash to to_h ([#2351](https://github.com/getsentry/sentry-ruby/pull/2351)) +- Query strings and form data in requests are no longer controlled by `send_default_pii` ([#2452](https://github.com/getsentry/sentry-ruby/pull/2452)) ## Unreleased diff --git a/sentry-ruby/lib/sentry/faraday.rb b/sentry-ruby/lib/sentry/faraday.rb index 05d5f45bd..2dffd59e3 100644 --- a/sentry-ruby/lib/sentry/faraday.rb +++ b/sentry-ruby/lib/sentry/faraday.rb @@ -57,13 +57,8 @@ def instrument(op_name, env, &block) def extract_request_info(env) url = env[:url].scheme + "://" + env[:url].host + env[:url].path - result = { method: env[:method].to_s.upcase, url: url } - - if Sentry.configuration.send_default_pii - result[:query] = env[:url].query - result[:body] = env[:body] - end - + result = { method: env[:method].to_s.upcase, url: url, query: env[:url].query } + result[:body] = env[:body] if Sentry.configuration.send_default_pii result end end diff --git a/sentry-ruby/lib/sentry/interfaces/request.rb b/sentry-ruby/lib/sentry/interfaces/request.rb index 4647e6562..c44dbbfa3 100644 --- a/sentry-ruby/lib/sentry/interfaces/request.rb +++ b/sentry-ruby/lib/sentry/interfaces/request.rb @@ -53,14 +53,12 @@ def initialize(env:, send_default_pii:, rack_env_whitelist:) request = ::Rack::Request.new(env) - if send_default_pii - self.data = read_data_from(request) - self.cookies = request.cookies - self.query_string = request.query_string - end - self.url = request.scheme && request.url.split("?").first self.method = request.request_method + self.data = read_data_from(request) + self.query_string = request.query_string + + self.cookies = request.cookies if send_default_pii self.headers = filter_and_format_headers(env, send_default_pii) self.env = filter_and_format_env(env, rack_env_whitelist) diff --git a/sentry-ruby/lib/sentry/net/http.rb b/sentry-ruby/lib/sentry/net/http.rb index 04820f3d5..51039c81b 100644 --- a/sentry-ruby/lib/sentry/net/http.rb +++ b/sentry-ruby/lib/sentry/net/http.rb @@ -69,12 +69,8 @@ def extract_request_info(req) uri = req.uri || URI.parse(URI::DEFAULT_PARSER.escape("#{use_ssl? ? 'https' : 'http'}://#{hostname}#{req.path}")) url = "#{uri.scheme}://#{uri.host}#{uri.path}" rescue uri.to_s - result = { method: req.method, url: url } - - if Sentry.configuration.send_default_pii - result[:query] = uri.query - result[:body] = req.body - end + result = { method: req.method, url: url, query: uri.query } + result[:body] = req.body if Sentry.configuration.send_default_pii result end diff --git a/sentry-ruby/spec/sentry/breadcrumb/http_logger_spec.rb b/sentry-ruby/spec/sentry/breadcrumb/http_logger_spec.rb index a4fab84ed..cdd3ec76a 100644 --- a/sentry-ruby/spec/sentry/breadcrumb/http_logger_spec.rb +++ b/sentry-ruby/spec/sentry/breadcrumb/http_logger_spec.rb @@ -63,7 +63,7 @@ Sentry.configuration.send_default_pii = false end - it "adds http breadcrumbs without query string & request body" do + it "adds http breadcrumbs without request body" do stub_normal_response response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar")) @@ -71,7 +71,7 @@ expect(response.code).to eq("200") crumb = Sentry.get_current_scope.breadcrumbs.peek expect(crumb.category).to eq("net.http") - expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" }) + expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" }) http = Net::HTTP.new("example.com") request = Net::HTTP::Get.new("/path?foo=bar") @@ -80,7 +80,7 @@ expect(response.code).to eq("200") crumb = Sentry.get_current_scope.breadcrumbs.peek expect(crumb.category).to eq("net.http") - expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" }) + expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" }) request = Net::HTTP::Post.new("/path?foo=bar") request.body = 'quz=qux' @@ -89,7 +89,7 @@ expect(response.code).to eq("200") crumb = Sentry.get_current_scope.breadcrumbs.peek expect(crumb.category).to eq("net.http") - expect(crumb.data).to eq({ status: 200, method: "POST", url: "http://example.com/path" }) + expect(crumb.data).to eq({ status: 200, method: "POST", query: "foo=bar", url: "http://example.com/path" }) end end @@ -115,7 +115,7 @@ expect(response.code).to eq("200") crumb = Sentry.get_current_scope.breadcrumbs.peek expect(crumb.category).to eq("net.http") - expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" }) + expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" }) end end end diff --git a/sentry-ruby/spec/sentry/event_spec.rb b/sentry-ruby/spec/sentry/event_spec.rb index e384f3673..568bbeef8 100644 --- a/sentry-ruby/spec/sentry/event_spec.rb +++ b/sentry-ruby/spec/sentry/event_spec.rb @@ -104,6 +104,8 @@ headers: { 'Host' => 'localhost', 'X-Request-Id' => 'abcd-1234-abcd-1234' }, method: 'POST', url: 'http://localhost/lol', + query_string: 'biz=baz', + data: { 'foo' => 'bar' } ) expect(event.to_h[:tags][:request_id]).to eq("abcd-1234-abcd-1234") expect(event.to_h[:user][:ip_address]).to eq(nil) diff --git a/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb b/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb index 14dde530f..8587887f6 100644 --- a/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb +++ b/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb @@ -139,22 +139,22 @@ def to_s end end - it "doesn't store request body by default" do - env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=ignore me")) + it "stores form data" do + env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=catch me")) - expect(subject.data).to eq(nil) + expect(subject.data).to eq({ "data" => "catch me" }) end - it "doesn't store request body by default" do - env.merge!(::Rack::RACK_INPUT => StringIO.new("ignore me")) + it "stores query string" do + env.merge!("QUERY_STRING" => "token=xxxx") - expect(subject.data).to eq(nil) + expect(subject.query_string).to eq("token=xxxx") end - it "doesn't store query_string by default" do - env.merge!("QUERY_STRING" => "token=xxxx") + it "stores request body" do + env.merge!(::Rack::RACK_INPUT => StringIO.new("catch me")) - expect(subject.query_string).to eq(nil) + expect(subject.data).to eq("catch me") end context "with config.send_default_pii = true" do @@ -166,24 +166,6 @@ def to_s expect(subject.cookies).to eq("cookies!") end - it "stores form data" do - env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=catch me")) - - expect(subject.data).to eq({ "data" => "catch me" }) - end - - it "stores query string" do - env.merge!("QUERY_STRING" => "token=xxxx") - - expect(subject.query_string).to eq("token=xxxx") - end - - it "stores request body" do - env.merge!(::Rack::RACK_INPUT => StringIO.new("catch me")) - - expect(subject.data).to eq("catch me") - end - it "stores Authorization header" do env.merge!("HTTP_AUTHORIZATION" => "Basic YWxhZGRpbjpvcGVuc2VzYW1l") diff --git a/sentry-ruby/spec/sentry/net/http_spec.rb b/sentry-ruby/spec/sentry/net/http_spec.rb index 9de0a6f76..94dc6b6f9 100644 --- a/sentry-ruby/spec/sentry/net/http_spec.rb +++ b/sentry-ruby/spec/sentry/net/http_spec.rb @@ -46,67 +46,30 @@ end end - context "with config.send_default_pii = true" do - before do - Sentry.configuration.send_default_pii = true - end - - it "records the request's span with query string in data" do - stub_normal_response - - transaction = Sentry.start_transaction - Sentry.get_current_scope.set_span(transaction) - - response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar")) - - expect(response.code).to eq("200") - expect(transaction.span_recorder.spans.count).to eq(2) - - request_span = transaction.span_recorder.spans.last - expect(request_span.op).to eq("http.client") - expect(request_span.origin).to eq("auto.http.net_http") - expect(request_span.start_timestamp).not_to be_nil - expect(request_span.timestamp).not_to be_nil - expect(request_span.start_timestamp).not_to eq(request_span.timestamp) - expect(request_span.description).to eq("GET http://example.com/path") - expect(request_span.data).to eq({ - "http.response.status_code" => 200, - "url" => "http://example.com/path", - "http.request.method" => "GET", - "http.query" => "foo=bar" - }) - end - end - - context "with config.send_default_pii = false" do - before do - Sentry.configuration.send_default_pii = false - end + it "records the request's span with query string in data" do + stub_normal_response - it "records the request's span without query string" do - stub_normal_response + transaction = Sentry.start_transaction + Sentry.get_current_scope.set_span(transaction) - transaction = Sentry.start_transaction - Sentry.get_current_scope.set_span(transaction) + response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar")) - response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar")) - - expect(response.code).to eq("200") - expect(transaction.span_recorder.spans.count).to eq(2) + expect(response.code).to eq("200") + expect(transaction.span_recorder.spans.count).to eq(2) - request_span = transaction.span_recorder.spans.last - expect(request_span.op).to eq("http.client") - expect(request_span.origin).to eq("auto.http.net_http") - expect(request_span.start_timestamp).not_to be_nil - expect(request_span.timestamp).not_to be_nil - expect(request_span.start_timestamp).not_to eq(request_span.timestamp) - expect(request_span.description).to eq("GET http://example.com/path") - expect(request_span.data).to eq({ - "http.response.status_code" => 200, - "url" => "http://example.com/path", - "http.request.method" => "GET" - }) - end + request_span = transaction.span_recorder.spans.last + expect(request_span.op).to eq("http.client") + expect(request_span.origin).to eq("auto.http.net_http") + expect(request_span.start_timestamp).not_to be_nil + expect(request_span.timestamp).not_to be_nil + expect(request_span.start_timestamp).not_to eq(request_span.timestamp) + expect(request_span.description).to eq("GET http://example.com/path") + expect(request_span.data).to eq({ + "http.response.status_code" => 200, + "url" => "http://example.com/path", + "http.request.method" => "GET", + "http.query" => "foo=bar" + }) end it "supports non-ascii characters in the path" do @@ -348,7 +311,8 @@ def verify_spans(transaction) expect(request_span.data).to eq({ "http.response.status_code" => 200, "url" => "http://example.com/path", - "http.request.method" => "GET" + "http.request.method" => "GET", + "http.query" => "foo=bar" }) request_span = transaction.span_recorder.spans[2] @@ -361,7 +325,8 @@ def verify_spans(transaction) expect(request_span.data).to eq({ "http.response.status_code" => 404, "url" => "http://example.com/path", - "http.request.method" => "GET" + "http.request.method" => "GET", + "http.query" => "foo=bar" }) end