Don't gate request query strings and form data behind send_default_pii

Author: sl0thentr0py

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
 

sentry-ruby/lib/sentry/faraday.rb

@@ -56,15 +56,12 @@ def instrument(op_name, env, &block)
       private
 
       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: env[:url].scheme + "://" + env[:url].host + env[:url].path,
+          query: env[:url].query,
+          body: env[:body]
+        }
       end
     end
   end

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)

sentry-ruby/lib/sentry/net/http.rb

@@ -69,14 +69,12 @@ 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,
+          body: req.body
+        }
       end
     end
   end

sentry-ruby/spec/sentry/breadcrumb/http_logger_spec.rb

@@ -63,15 +63,15 @@
       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"))
 
       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

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)

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")
 

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