feat(data-collection): Request data collection

sentry-ruby/lib/sentry/configuration.rb

@@ -212,6 +212,7 @@ class Configuration
     attr_accessor :propagate_traces
 
     # Array of rack env parameters to be included in the event sent to sentry.
+    # @deprecated Use `data_collection.http_headers.request` to control request data collection instead.
     # @return [Array<String>]
     attr_accessor :rack_env_whitelist
 
@@ -826,6 +827,7 @@ def log_deprecations
       log_warn("`send_default_pii` is deprecated; use `data_collection` instead.") if self.send_default_pii
       log_warn("`include_local_variables` is deprecated; use `data_collection.stack_frame_variables` instead.") if include_local_variables
       log_warn("`context_lines` is deprecated; use `data_collection.frame_context_lines` instead.") if context_lines != 3
+      log_warn("`rack_env_whitelist` is deprecated; use `data_collection.http_headers.request` to control request data collection instead.") if rack_env_whitelist != RACK_ENV_WHITELIST_DEFAULT
     end
 
     # @api private

sentry-ruby/lib/sentry/data_collection.rb

@@ -28,7 +28,11 @@
     #     data_collection.stack_frame_variables = true
     #     data_collection.frame_context_lines = 5
     #   end
+
     MODES = %i[off deny_list allow_list].freeze
+
+    PII_HEADER_SNIPPETS = %w[forwarded -ip _ip remote via _user -user].freeze
+
     BODY_TYPES = %i[
     incoming_request
     outgoing_request
@@ -102,6 +106,11 @@
     # @default `3`
     attr_accessor :frame_context_lines
 
+    # Filters key-value data using the default sensitive denylist.
+    def self.filter(values)
+      KeyValueCollection.new(mode: :deny_list, terms: nil).filter(values)
+    end
+
     # Builds data collection settings compatible with the legacy send_default_pii
     # configuration.
     def self.backfill(configuration)
@@ -112,8 +121,10 @@
       # TODO-neel-data map to exact ruby behaviour for backwards compat behavior
       data_collection.user_info = false
       data_collection.cookies.mode = :off
-      data_collection.http_headers.request.mode = :off
-      data_collection.http_headers.response.mode = :off
+      data_collection.http_headers.request.mode = :deny_list
+      data_collection.http_headers.request.terms = PII_HEADER_SNIPPETS
+      data_collection.http_headers.response.mode = :deny_list
+      data_collection.http_headers.response.terms = PII_HEADER_SNIPPETS
       data_collection.http_bodies = []
       data_collection.url_query_params.mode = :off
       data_collection.graphql.document = false
@@ -132,13 +143,19 @@
         request: KeyValueCollection.new(mode: :deny_list, terms: nil),
         response: KeyValueCollection.new(mode: :deny_list, terms: nil)
       )
-      @http_bodies = nil
+      @http_bodies = BODY_TYPES
       @url_query_params = KeyValueCollection.new(mode: :deny_list, terms: nil)
       @database_query_data = true
       @graphql = GraphQL.new(document: true, variables: true)
       @queues = true
       @stack_frame_variables = false
       @frame_context_lines = 3
     end
+
+    # Returns whether incoming HTTP request bodies should be collected.
+    # nil imples all BODY_TYPES according to spec
+    def collect_incoming_http_body?
+      http_bodies.nil? || http_bodies.include?(:incoming_request)
+    end
   end
 end

sentry-ruby/lib/sentry/data_collection/key_value_collection.rb

@@ -7,7 +7,6 @@ class KeyValueCollection
       FILTERED_VALUE = "[Filtered]"
 
       # Keys from this list are ALWAYS filtered, regardless of :mode
-      # TODO-neel-data cookies have separate list, see JS
       SENSITIVE_DENY_LIST = %w[
         auth
         token
@@ -30,6 +29,30 @@ class KeyValueCollection
         set-cookie
       ].freeze
 
+      # Additional terms applied to cookie names only. These cover common
+      # opaque session, identity-provider, and load-balancer cookies without
+      # making the general header denylist overly broad.
+      SENSITIVE_COOKIE_NAME_DENY_LIST = %w[
+        .sid
+        sessid
+        remember
+        oidc
+        pkce
+        nonce
+        __secure-
+        __host-
+        awsalb
+        awselb
+        akamai
+        __stripe
+        cognito
+        firebase
+        supabase
+        sb-
+        mfa
+        2fa
+      ].freeze
+
       # `mode` controls whether values are collected:
       # - `:off` disables collection.
       # - `:deny_list` collects values except those matching `terms`.
@@ -56,19 +79,19 @@ def terms=(terms)
       #
       # @param values [Hash] key-value data to filter
       # @return [Hash] a new filtered hash, or an empty hash when collection is off
-      def filter(values)
+      def filter(values, cookie: false)
         return {} if mode == :off
 
         values.each_with_object({}) do |(key, value), filtered|
-          filtered[key] = safe_value?(key) ? value : FILTERED_VALUE
+          filtered[key] = safe_value?(key, cookie: cookie) ? value : FILTERED_VALUE
         end
       end
 
       private
 
-      def safe_value?(key)
+      def safe_value?(key, cookie: false)
         key_downcase = key.to_s.downcase
-        return false if sensitive?(key_downcase)
+        return false if sensitive?(key_downcase, cookie: cookie)
 
         case mode
         when :deny_list
@@ -80,8 +103,9 @@ def safe_value?(key)
         end
       end
 
-      def sensitive?(key)
-        SENSITIVE_DENY_LIST.any? { |term| key.include?(term) }
+      def sensitive?(key, cookie: false)
+        SENSITIVE_DENY_LIST.any? { |term| key.include?(term) } ||
+          (cookie && SENSITIVE_COOKIE_NAME_DENY_LIST.any? { |term| key.include?(term) })
       end
 
       def matches_any_term?(key)

sentry-ruby/lib/sentry/event.rb

@@ -28,7 +28,7 @@ class Event
 
     MAX_MESSAGE_SIZE_IN_BYTES = 1024 * 8
 
-    SKIP_INSPECTION_ATTRIBUTES = [:@modules, :@stacktrace_builder, :@send_default_pii, :@data_collection, :@trusted_proxies, :@rack_env_whitelist]
+    SKIP_INSPECTION_ATTRIBUTES = [:@modules, :@stacktrace_builder, :@data_collection, :@trusted_proxies, :@rack_env_whitelist]
 
     include CustomInspection
 
@@ -73,7 +73,6 @@ def initialize(configuration:, integration_meta: nil, message: nil)
       @modules = configuration.gem_specs if configuration.send_modules
 
       # configuration options to help events process data
-      @send_default_pii = configuration.send_default_pii
       @data_collection = configuration.data_collection
       @trusted_proxies = configuration.trusted_proxies
       @stacktrace_builder = configuration.stacktrace_builder
@@ -128,7 +127,11 @@ def to_json_compatible
     private
 
     def add_request_interface(env)
-      @request = Sentry::RequestInterface.new(env: env, send_default_pii: @send_default_pii, rack_env_whitelist: @rack_env_whitelist)
+      @request = Sentry::RequestInterface.new(
+        env: env,
+        data_collection: @data_collection,
+        rack_env_whitelist: @rack_env_whitelist
+      )
     end
 
     def serialize_attributes

sentry-ruby/lib/sentry/interfaces/request.rb

@@ -1,15 +1,11 @@
 # frozen_string_literal: true
 
+require "json"
+
 module Sentry
   class RequestInterface < Interface
     REQUEST_ID_HEADERS = %w[action_dispatch.request_id HTTP_X_REQUEST_ID].freeze
     CONTENT_HEADERS = %w[CONTENT_TYPE CONTENT_LENGTH].freeze
-    IP_HEADERS = [
-      "REMOTE_ADDR",
-      "HTTP_CLIENT_IP",
-      "HTTP_X_REAL_IP",
-      "HTTP_X_FORWARDED_FOR"
-    ].freeze
 
     # Regex to detect lowercase chars — match? is allocation-free (no MatchData/String)
     LOWERCASE_PATTERN = /[a-z]/.freeze
@@ -27,10 +23,10 @@ class RequestInterface < Interface
     # @return [Hash]
     attr_accessor :data
 
-    # @return [String]
+    # @return [String, Hash]
     attr_accessor :query_string
 
-    # @return [String]
+    # @return [Hash]
     attr_accessor :cookies
 
     # @return [Hash]
@@ -40,33 +36,24 @@ class RequestInterface < Interface
     attr_accessor :env
 
     # @param env [Hash]
-    # @param send_default_pii [Boolean]
+    # @param data_collection [DataCollection]
+    # @param send_default_pii [Boolean] Deprecated compatibility input, unused.
     # @param rack_env_whitelist [Array]
+    # @see Configuration#data_collection
     # @see Configuration#send_default_pii
     # @see Configuration#rack_env_whitelist
-    def initialize(env:, send_default_pii:, rack_env_whitelist:)
+    def initialize(env:, data_collection:, rack_env_whitelist:, send_default_pii: nil)
       env = env.dup
-
-      unless send_default_pii
-        # need to completely wipe out ip addresses
-        RequestInterface::IP_HEADERS.each do |header|
-          env.delete(header)
-        end
-      end
-
       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.headers = filter_and_format_headers(env, send_default_pii)
-      self.env     = filter_and_format_env(env, rack_env_whitelist)
+      query = data_collection.url_query_params.filter(request.GET) rescue nil
+
+      self.method       = request.request_method
+      self.url          = request.scheme && request.url.split("?").first
+      self.query_string = query unless query&.empty?
+      self.cookies      = data_collection.cookies.filter(request.cookies, cookie: true)
+      self.data         = read_data_from(request) if data_collection.collect_incoming_http_body?
+      self.headers      = filter_and_format_headers(env, data_collection.http_headers.request)
+      self.env          = filter_and_format_env(env, data_collection.http_headers.request, rack_env_whitelist)
     end
 
     private
@@ -75,25 +62,31 @@ def read_data_from(request)
       return "Skipped non-rewindable request body" unless request.body.respond_to?(:rewind)
 
       if request.form_data?
-        request.POST
-      elsif request.body # JSON requests, etc
-        data = request.body.read(MAX_BODY_LIMIT)
-        data = Utils::EncodingHelper.encode_to_utf_8(data.to_s)
-        request.body.rewind
-        data
+        DataCollection.filter(request.POST)
+      else
+        body = request.body.read(MAX_BODY_LIMIT)
+        body = Utils::EncodingHelper.encode_to_utf_8(body.to_s)
+
+        if request.media_type == "application/json" || request.media_type&.end_with?("+json")
+          parsed_body = JSON.parse(body)
+          parsed_body.is_a?(Hash) ? DataCollection.filter(parsed_body) : parsed_body
+        else
+          body
+        end
       end
-    rescue IOError => e
+    rescue JSON::ParserError, IOError => e
       e.message
+    ensure
+      request.body.rewind if request.body.respond_to?(:rewind)
     end
 
-    def filter_and_format_headers(env, send_default_pii)
+    def filter_and_format_headers(env, collection)
       env.each_with_object({}) do |(key, value), memo|
         begin
           key = key.to_s # rack env can contain symbols
           next memo["X-Request-Id"] ||= Utils::RequestId.read_from(env) if Utils::RequestId::REQUEST_ID_HEADERS.include?(key)
           next if is_server_protocol?(key, value, env["SERVER_PROTOCOL"])
           next if is_skippable_header?(key)
-          next if key == "HTTP_AUTHORIZATION" && !send_default_pii
 
           # Rack stores headers as HTTP_WHAT_EVER, we need What-Ever
           key = key.delete_prefix("HTTP_")
@@ -107,12 +100,13 @@ def filter_and_format_headers(env, send_default_pii)
           Sentry.sdk_logger.warn(LOGGER_PROGNAME) { "Error raised while formatting headers: #{e.message}" }
           next
         end
+      end.then do |e|
+        collection.filter(e)
       end
     end
 
     def is_skippable_header?(key)
       key.match?(LOWERCASE_PATTERN) || # lower-case envs aren't real http headers
-        key == "HTTP_COOKIE" || # Cookies don't go here, they go somewhere else
         !(key.start_with?("HTTP_") || CONTENT_HEADERS.include?(key))
     end
 
@@ -134,11 +128,15 @@ def self.rack_3_or_above?
         Gem::Version.new(::Rack.release) >= Gem::Version.new("3.0")
     end
 
-    def filter_and_format_env(env, rack_env_whitelist)
-      return env if rack_env_whitelist.empty?
-
-      env.select do |k, _v|
-        rack_env_whitelist.include? k.to_s
+    def filter_and_format_env(env, collection, rack_env_whitelist)
+      if rack_env_whitelist.empty?
+        env
+      else
+        env.select do |k, _v|
+          rack_env_whitelist.include? k.to_s
+        end
+      end.then do |e|
+        collection.filter(e)
       end
     end
   end

sentry-ruby/spec/sentry/client/event_sending_spec.rb

@@ -348,6 +348,21 @@
         end
       end
 
+      context "when scope contains a malformed query string", when: :rack_available? do
+        before do
+          configuration.background_worker_threads = 0
+          stub_request(:post, "http://sentry.localdomain/sentry/api/42/envelope/").to_return(status: 200)
+          env = Rack::MockRequest.env_for("/test")
+          env["QUERY_STRING"] = "a=%"
+          scope.set_rack_env(env)
+        end
+
+        it "still captures the event" do
+          expect(client.capture_event(event, scope)).to eq(event)
+          expect(event.request.query_string).to be_nil
+        end
+      end
+
       context "when scope.apply_to_event fails" do
         before do
           scope.add_event_processor do

sentry-ruby/spec/sentry/configuration_spec.rb

@@ -39,6 +39,15 @@
 
       expect(subject).to have_received(:log_warn).with("`context_lines` is deprecated; use `data_collection.frame_context_lines` instead.")
     end
+
+    it "warns when rack_env_whitelist is changed" do
+      allow(subject).to receive(:log_warn)
+      subject.rack_env_whitelist = ["REMOTE_ADDR"]
+
+      subject.send(:log_deprecations)
+
+      expect(subject).to have_received(:log_warn).with("`rack_env_whitelist` is deprecated; use `data_collection.http_headers.request` to control request data collection instead.")
+    end
   end
 
   describe "#background_worker_threads" do

sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb

@@ -57,7 +57,7 @@
       it "normalizes terms assigned after initialization" do
         collection.terms = ["USER"]
 
-        expect(collection.filter("user_id" => "1")).to eq("user_id" => "[Filtered]")
+        expect(collection.filter({ "user_id" => "1" })).to eq("user_id" => "[Filtered]")
       end
 
       it "covers every built-in sensitive term" do