feat(data-collection): Port redis, net:http, faraday and excon

sentry-ruby/lib/sentry/data_collection.rb

@@ -157,9 +157,15 @@ def initialize
     end
 
     # Returns whether incoming HTTP request bodies should be collected.
-    # nil imples all BODY_TYPES according to spec
+    # nil implies all BODY_TYPES according to spec
     def collect_incoming_http_body?
       http_bodies.nil? || http_bodies.include?(:incoming_request)
     end
+
+    # Returns whether outgoing HTTP request bodies should be collected.
+    # nil implies all BODY_TYPES according to spec
+    def collect_outgoing_http_body?
+      http_bodies.nil? || http_bodies.include?(:outgoing_request)
+    end
   end
 end

sentry-ruby/lib/sentry/excon/middleware.rb

@@ -61,14 +61,9 @@ def extract_request_info(env)
         url = env[:scheme] + "://" + env[:hostname] + env[:path]
         result = { method: env[:method].to_s.upcase, url: url }
 
-        if Sentry.configuration.send_default_pii
-          result[:query] = env[:query]
-
-          # Handle excon 1.0.0+
-          result[:query] = build_nested_query(result[:query]) unless result[:query].is_a?(String)
-
-          result[:body] = env[:body]
-        end
+        query = filter_query_params(env[:query])
+        result[:query] = query if query
+        result[:body] = env[:body] if Sentry.configuration.data_collection.collect_outgoing_http_body?
 
         result
       end

sentry-ruby/lib/sentry/faraday.rb

@@ -59,10 +59,9 @@ 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
+        query = filter_query_params(env[:url].query)
+        result[:query] = query if query
+        result[:body] = env[:body] if Sentry.configuration.data_collection.collect_outgoing_http_body?
 
         result
       end

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

@@ -72,10 +72,9 @@ def extract_request_info(req)
 
         result = { method: req.method, url: url }
 
-        if Sentry.configuration.send_default_pii
-          result[:query] = uri.query
-          result[:body] = req.body
-        end
+        query = filter_query_params(uri.query)
+        result[:query] = query if query
+        result[:body] = req.body if Sentry.configuration.data_collection.collect_outgoing_http_body?
 
         result
       end

sentry-ruby/lib/sentry/redis.rb

@@ -62,7 +62,7 @@ def parsed_commands
         command_set = { command: command.to_s.upcase }
         command_set[:key] = key if Utils::EncodingHelper.valid_utf_8?(key)
 
-        if Sentry.configuration.send_default_pii
+        if Sentry.configuration.data_collection.database_query_data
           command_set[:arguments] = arguments
                                     .select { |a| Utils::EncodingHelper.valid_utf_8?(a) }
                                     .join(" ")

sentry-ruby/lib/sentry/utils/http_tracing.rb

@@ -1,11 +1,39 @@
 # frozen_string_literal: true
 
+require "uri"
+
 module Sentry
   module Utils
     module HttpTracing
+      def filter_query_params(query)
+        return nil unless query
+        return nil unless query.is_a?(String) || query.is_a?(Hash)
+
+        query_hash = if query.is_a?(String)
+          URI.decode_www_form(query).each_with_object({}) do |(key, value), params|
+            params[key] = params.key?(key) ? Array(params[key]) + [value] : value
+          end
+        else
+          query
+        end
+
+        filtered_query_hash = Sentry.configuration.data_collection.url_query_params.filter(query_hash)
+        return nil if filtered_query_hash.empty?
+
+        format_query(filtered_query_hash)
+      rescue
+        nil
+      end
+
+      def format_query(query)
+        query.flat_map do |key, value|
+          Array(value).map { |item| "#{key}=#{item}" }
+        end.join("&")
+      end
+
       def set_span_info(sentry_span, request_info, response_status)
         sentry_span.set_description("#{request_info[:method]} #{request_info[:url]}")
-        sentry_span.set_data(Span::DataConventions::URL, request_info[:url])
+        sentry_span.set_data(Span::DataConventions::URL, url_with_query(request_info))
         sentry_span.set_data(Span::DataConventions::HTTP_METHOD, request_info[:method])
         sentry_span.set_data(Span::DataConventions::HTTP_QUERY, request_info[:query]) if request_info[:query]
         sentry_span.set_data(Span::DataConventions::HTTP_STATUS_CODE, response_status)
@@ -43,37 +71,25 @@ def propagate_trace?(url)
           Sentry.configuration.trace_propagation_targets.any? { |target| url.match?(target) }
       end
 
-      # Kindly borrowed from Rack::Utils
-      def build_nested_query(value, prefix = nil)
-        case value
-        when Array
-          value.map { |v|
-            build_nested_query(v, "#{prefix}[]")
-          }.join("&")
-        when Hash
-          value.map { |k, v|
-            build_nested_query(v, prefix ? "#{prefix}[#{k}]" : k)
-          }.delete_if(&:empty?).join("&")
-        when nil
-          URI.encode_www_form_component(prefix)
-        else
-          raise ArgumentError, "value must be a Hash" if prefix.nil?
-          "#{URI.encode_www_form_component(prefix)}=#{URI.encode_www_form_component(value)}"
-        end
-      end
 
       private
 
+      def url_with_query(request_info)
+        return request_info[:url] unless request_info[:query]
+
+        "#{request_info[:url]}?#{request_info[:query]}"
+      end
+
       def get_level(status)
         return :info unless status && status.is_a?(Integer)
 
-if status >= 500
-  :error
-elsif status >= 400
-  :warning
-else
-  :info
-end
+        if status >= 500
+          :error
+        elsif status >= 400
+          :warning
+        else
+          :info
+        end
       end
     end
   end

sentry-ruby/spec/sentry/excon_spec.rb

@@ -56,6 +56,85 @@
       end
     end
 
+    describe "data collection" do
+      describe "query parameters" do
+        def perform_get_request(query: "token=secret&page=5")
+          transaction = Sentry.start_transaction
+          Sentry.get_current_scope.set_span(transaction)
+          Excon.stub({}, { body: "", status: 200 })
+          Excon.get("http://example.com/path?#{query}", mock: true)
+          transaction
+        end
+
+        it "does not collect them when the mode is off" do
+          Sentry.configuration.data_collection.url_query_params.mode = :off
+          transaction = perform_get_request
+
+          expect(transaction.span_recorder.spans.last.data).not_to have_key("http.query")
+          expect(transaction.span_recorder.spans.last.data["url"]).to eq("http://example.com/path")
+        end
+
+        it "filters sensitive values in deny-list mode" do
+          Sentry.configuration.data_collection.url_query_params.mode = :deny_list
+          transaction = perform_get_request
+
+          expect(transaction.span_recorder.spans.last.data["http.query"]).to eq(
+            "token=[Filtered]&page=5"
+          )
+          expect(transaction.span_recorder.spans.last.data["url"]).to eq(
+            "http://example.com/path?token=[Filtered]&page=5"
+          )
+        end
+
+        it "collects only allowed values in allow-list mode" do
+          Sentry.configuration.data_collection.url_query_params.mode = :allow_list
+          Sentry.configuration.data_collection.url_query_params.terms = ["page"]
+          transaction = perform_get_request(query: "another=value&page=5")
+
+          expect(transaction.span_recorder.spans.last.data["http.query"]).to eq(
+            "another=[Filtered]&page=5"
+          )
+          expect(transaction.span_recorder.spans.last.data["url"]).to eq(
+            "http://example.com/path?another=[Filtered]&page=5"
+          )
+        end
+      end
+
+      describe "request bodies" do
+        before do
+          Sentry.configuration.breadcrumbs_logger = [:http_logger]
+        end
+
+        def perform_post_request
+          transaction = Sentry.start_transaction
+          Sentry.get_current_scope.set_span(transaction)
+          Excon.stub({}, { body: "", status: 200 })
+          Excon.post("http://example.com/path", body: "secret body", mock: true)
+        end
+
+        it "collects them when all body types are enabled" do
+          Sentry.configuration.data_collection.http_bodies = nil
+          perform_post_request
+
+          expect(Sentry.get_current_scope.breadcrumbs.peek.data[:body]).to eq("secret body")
+        end
+
+        it "collects them when outgoing requests are enabled" do
+          Sentry.configuration.data_collection.http_bodies = [:outgoing_request]
+          perform_post_request
+
+          expect(Sentry.get_current_scope.breadcrumbs.peek.data[:body]).to eq("secret body")
+        end
+
+        it "does not collect them when body types are disabled" do
+          Sentry.configuration.data_collection.http_bodies = []
+          perform_post_request
+
+          expect(Sentry.get_current_scope.breadcrumbs.peek.data).not_to have_key(:body)
+        end
+      end
+    end
+
     context "with config.send_default_pii = true" do
       before do
         Sentry.configuration.send_default_pii = true
@@ -82,7 +161,7 @@
         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",
+          "url" => "http://example.com/path?foo=bar",
           "http.request.method" => "GET",
           "http.query" => "foo=bar"
         })
@@ -95,7 +174,7 @@
         Sentry.get_current_scope.set_span(transaction)
 
         connection = Excon.new("http://example.com/path")
-        response = connection.get(mock: true, query: build_nested_query({ foo: "bar", baz: [1, 2], qux: { a: 1, b: 2 } }))
+        response = connection.get(mock: true, query: "foo=bar&baz[]=1&baz[]=2&qux[a]=1&qux[b]=2")
 
         expect(response.status).to eq(200)
         expect(transaction.span_recorder.spans.count).to eq(2)
@@ -109,9 +188,9 @@
         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",
+          "url" => "http://example.com/path?foo=bar&baz[]=1&baz[]=2&qux[a]=1&qux[b]=2",
           "http.request.method" => "GET",
-          "http.query" => "foo=bar&baz%5B%5D=1&baz%5B%5D=2&qux%5Ba%5D=1&qux%5Bb%5D=2"
+          "http.query" => "foo=bar&baz[]=1&baz[]=2&qux[a]=1&qux[b]=2"
         })
       end
 

sentry-ruby/spec/sentry/faraday_spec.rb

@@ -57,6 +57,95 @@
       end
     end
 
+    describe "data collection" do
+      describe "query parameters" do
+        let(:http) do
+          Faraday.new("http://example.com") do |f|
+            f.adapter Faraday::Adapter::Test do |stub|
+              stub.get("/test") { [200, {}, ""] }
+            end
+          end
+        end
+
+        it "does not collect them when the mode is off" do
+          Sentry.configuration.data_collection.url_query_params.mode = :off
+          transaction = Sentry.start_transaction
+          Sentry.get_current_scope.set_span(transaction)
+
+          http.get("/test?token=secret&page=5")
+
+          expect(transaction.span_recorder.spans.last.data).not_to have_key("http.query")
+          expect(transaction.span_recorder.spans.last.data["url"]).to eq("http://example.com/test")
+        end
+
+        it "filters sensitive values in deny-list mode" do
+          Sentry.configuration.data_collection.url_query_params.mode = :deny_list
+          transaction = Sentry.start_transaction
+          Sentry.get_current_scope.set_span(transaction)
+
+          http.get("/test?token=secret&page=5")
+
+          expect(transaction.span_recorder.spans.last.data["http.query"]).to eq(
+            "page=5&token=[Filtered]"
+          )
+          expect(transaction.span_recorder.spans.last.data["url"]).to eq(
+            "http://example.com/test?page=5&token=[Filtered]"
+          )
+        end
+
+        it "collects only allowed values in allow-list mode" do
+          Sentry.configuration.data_collection.url_query_params.mode = :allow_list
+          Sentry.configuration.data_collection.url_query_params.terms = ["page"]
+          transaction = Sentry.start_transaction
+          Sentry.get_current_scope.set_span(transaction)
+
+          http.get("/test?another=value&page=5")
+
+          expect(transaction.span_recorder.spans.last.data["http.query"]).to eq(
+            "another=[Filtered]&page=5"
+          )
+          expect(transaction.span_recorder.spans.last.data["url"]).to eq(
+            "http://example.com/test?another=[Filtered]&page=5"
+          )
+        end
+      end
+
+      describe "request bodies" do
+        let(:http) do
+          Faraday.new("http://example.com") do |f|
+            f.adapter Faraday::Adapter::Test do |stub|
+              stub.post("/test") { [200, {}, ""] }
+            end
+          end
+        end
+
+        before do
+          Sentry.configuration.breadcrumbs_logger = [:http_logger]
+        end
+
+        it "collects them when all body types are enabled" do
+          Sentry.configuration.data_collection.http_bodies = nil
+          http.post("/test", "secret body")
+
+          expect(Sentry.get_current_scope.breadcrumbs.peek.data[:body]).to eq("secret body")
+        end
+
+        it "collects them when outgoing requests are enabled" do
+          Sentry.configuration.data_collection.http_bodies = [:outgoing_request]
+          http.post("/test", "secret body")
+
+          expect(Sentry.get_current_scope.breadcrumbs.peek.data[:body]).to eq("secret body")
+        end
+
+        it "does not collect them when body types are disabled" do
+          Sentry.configuration.data_collection.http_bodies = []
+          http.post("/test", "secret body")
+
+          expect(Sentry.get_current_scope.breadcrumbs.peek.data).not_to have_key(:body)
+        end
+      end
+    end
+
     context "with config.send_default_pii = true" do
       let(:http) do
         Faraday.new(url) do |f|
@@ -103,7 +192,7 @@
 
         expect(request_span.data).to eq({
           "http.response.status_code" => 200,
-          "url" => "http://example.com/test",
+          "url" => "http://example.com/test?foo=bar",
           "http.request.method" => "GET",
           "http.query" => "foo=bar"
         })
@@ -157,7 +246,7 @@
 
         expect(request_span.data).to eq({
           "http.response.status_code" => 200,
-          "url" => "http://example.com/test",
+          "url" => "http://example.com/test?foo=bar",
           "http.request.method" => "POST",
           "http.query" => "foo=bar"
         })