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)
* Use `data_collection.url_query_params` for query string and parse query into `Hash` and filter
* Use `data_collection.http_bodies` for body
* JSON is also parsed and filtered now, form data is also filtered for sensitive keys
* 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
@@ -102,6 +106,11 @@ def initialize(document:, variables:)
     # @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 @@ 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 +143,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

@@ -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,26 @@ 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, replaced with data_collection.
     # @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.url = request.scheme && request.url.split("?").first
       self.method = request.request_method
+      self.url = request.scheme && request.url.split("?").first
 
-      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)
+      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.http_bodies.include?(:incoming_request)
+      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 +64,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 +102,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 +130,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

@@ -3,15 +3,26 @@
 RSpec.describe Sentry::DataCollection do
   subject(:data_collection) { described_class.new }
 
+  describe ".filter" do
+    it "applies the default sensitive denylist" do
+      expect(described_class.filter("token" => "secret", "page" => "2")).to eq(
+        "token" => "[Filtered]",
+        "page" => "2"
+      )
+    end
+  end
+
   describe ".backfill" do
     it "uses the send_default_pii=false defaults" do
       configuration = Sentry::Configuration.new
       data_collection = described_class.backfill(configuration)
 
       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 +48,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
 
@@ -132,7 +129,7 @@
           env: { 'SERVER_NAME' => 'localhost', 'SERVER_PORT' => '80', "REMOTE_ADDR" => "192.168.1.1" },
           headers: { 'Host' => 'localhost', "X-Forwarded-For" => "1.1.1.1, 2.2.2.2", "X-Request-Id" => "abcd-1234-abcd-1234" },
           method: 'POST',
-          query_string: 'biz=baz',
+          query_string: { 'biz' => 'baz' },
           url: 'http://localhost/lol',
           cookies: {}
         )
@@ -160,7 +157,7 @@
             env: { 'SERVER_NAME' => 'localhost', 'SERVER_PORT' => '80', "REMOTE_ADDR" => "192.168.1.1" },
             headers: { 'Host' => 'localhost', "X-Forwarded-For" => "1.1.1.1, 2.2.2.2", "X-Request-Id" => "abcd-1234-abcd-1234" },
             method: 'POST',
-            query_string: 'biz=baz',
+            query_string: { 'biz' => 'baz' },
             url: 'http://localhost/lol',
             cookies: {}
           )