feat(data-collection): Request data collection

* Add `PII_HEADER_SNIPPETS` to backfill
* Use `data_collection.http_headers.request` for ip address collection
  (both `env` and `request`)
* Use `data_collection.cookies` for cookie collection (fix `Hash`
  signature since it always was one)
* Sensitive headers like auth and cookies are not *ALWAYS* filtered
  unlike before

Author: sl0thentr0py

sentry-ruby/lib/sentry/data_collection.rb

@@ -28,7 +28,11 @@ class DataCollection
     #     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
@@ -112,8 +116,10 @@ def self.backfill(configuration)
       # 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,7 +138,7 @@ def initialize
         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)

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

@@ -128,7 +128,12 @@ 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,
+        send_default_pii: @send_default_pii,
+        rack_env_whitelist: @rack_env_whitelist
+      )
     end
 
     def serialize_attributes

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

@@ -4,12 +4,6 @@ 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
@@ -30,7 +24,7 @@ class RequestInterface < Interface
     # @return [String]
     attr_accessor :query_string
 
-    # @return [String]
+    # @return [Hash]
     attr_accessor :cookies
 
     # @return [Hash]
@@ -40,33 +34,29 @@ 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; still gates legacy cookie,
+    #   body, query-string, and Authorization header collection until those categories are migrated.
     # @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:, send_default_pii:, rack_env_whitelist:)
       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.cookies = data_collection.cookies.filter(request.cookies, cookie: true)
       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)
+      collection = data_collection.http_headers.request
+      self.headers = filter_and_format_headers(env, collection)
+      self.env     = filter_and_format_env(env, collection, rack_env_whitelist)
     end
 
     private
@@ -86,14 +76,13 @@ def read_data_from(request)
       e.message
     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 +96,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 +124,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/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

sentry-ruby/spec/sentry/data_collection_spec.rb

@@ -10,8 +10,10 @@
 
       expect(data_collection.user_info).to eq(false)
       expect(data_collection.cookies.mode).to eq(:off)
-      expect(data_collection.http_headers.request.mode).to eq(:off)
-      expect(data_collection.http_headers.response.mode).to eq(:off)
+      expect(data_collection.http_headers.request.mode).to eq(:deny_list)
+      expect(data_collection.http_headers.request.terms).to eq(described_class::PII_HEADER_SNIPPETS)
+      expect(data_collection.http_headers.response.mode).to eq(:deny_list)
+      expect(data_collection.http_headers.request.terms).to eq(described_class::PII_HEADER_SNIPPETS)
       expect(data_collection.http_bodies).to eq([])
       expect(data_collection.url_query_params.mode).to eq(:off)
       expect(data_collection.database_query_data).to eq(false)
@@ -37,10 +39,10 @@
       expect(data_collection.cookies.mode).to eq(:deny_list)
       expect(data_collection.cookies.terms).to be_nil
       expect(data_collection.http_headers.request.mode).to eq(:deny_list)
-      expect(data_collection.http_headers.request.terms).to be_nil
+      expect(data_collection.http_headers.request.terms).to eq(nil)
       expect(data_collection.http_headers.response.mode).to eq(:deny_list)
-      expect(data_collection.http_headers.response.terms).to be_nil
-      expect(data_collection.http_bodies).to be_nil
+      expect(data_collection.http_headers.request.terms).to eq(nil)
+      expect(data_collection.http_bodies).to eq(described_class::BODY_TYPES)
       expect(data_collection.url_query_params.mode).to eq(:deny_list)
       expect(data_collection.url_query_params.terms).to be_nil
       expect(data_collection.database_query_data).to eq(true)

sentry-ruby/spec/sentry/event_spec.rb

@@ -98,24 +98,21 @@
         scope.apply_to_event(event)
 
         expect(event.to_h[:request]).to eq(
-          env: { 'SERVER_NAME' => 'localhost', 'SERVER_PORT' => '80' },
-          headers: { 'Host' => 'localhost', 'X-Request-Id' => 'abcd-1234-abcd-1234' },
+          env: { 'REMOTE_ADDR' => '[Filtered]', 'SERVER_NAME' => 'localhost', 'SERVER_PORT' => '80' },
+          headers: { 'Host' => 'localhost', 'X-Forwarded-For' => '[Filtered]', 'X-Request-Id' => 'abcd-1234-abcd-1234' },
           method: 'POST',
           url: 'http://localhost/lol',
+          cookies: {}
         )
         expect(event.to_h[:tags][:request_id]).to eq("abcd-1234-abcd-1234")
         expect(event.to_h[:user][:ip_address]).to eq(nil)
       end
 
-      it "removes ip address headers" do
+      it "filters ip address headers from headers and env" do
         scope.apply_to_event(event)
 
-        # doesn't affect scope's rack_env
-        expect(scope.rack_env).to include("REMOTE_ADDR")
-        expect(event.request.headers.keys).not_to include("REMOTE_ADDR")
-        expect(event.request.headers.keys).not_to include("Client-Ip")
-        expect(event.request.headers.keys).not_to include("X-Real-Ip")
-        expect(event.request.headers.keys).not_to include("X-Forwarded-For")
+        expect(event.request.env).to include("REMOTE_ADDR" => "[Filtered]")
+        expect(event.request.headers).to include("X-Forwarded-For" => "[Filtered]")
       end
     end