From bdfab9003849587aa94424c8ee9c8a443b6eea8b Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 22 Jul 2026 14:14:58 +0200 Subject: [PATCH] feat(data-collection): request data collection Co-Authored-By: OpenAI --- sentry-ruby/lib/sentry/configuration.rb | 2 + sentry-ruby/lib/sentry/data_collection.rb | 23 +- .../data_collection/key_value_collection.rb | 38 ++- sentry-ruby/lib/sentry/event.rb | 9 +- sentry-ruby/lib/sentry/interfaces/request.rb | 88 +++--- .../spec/sentry/client/event_sending_spec.rb | 15 + sentry-ruby/spec/sentry/configuration_spec.rb | 9 + .../key_value_collection_spec.rb | 2 +- .../spec/sentry/data_collection_spec.rb | 41 ++- sentry-ruby/spec/sentry/event_spec.rb | 19 +- .../interfaces/request_interface_spec.rb | 267 ++++++++++++++++-- .../sentry/rack/capture_exceptions_spec.rb | 4 +- 12 files changed, 416 insertions(+), 101 deletions(-) diff --git a/sentry-ruby/lib/sentry/configuration.rb b/sentry-ruby/lib/sentry/configuration.rb index 202046343..f3524ee63 100644 --- a/sentry-ruby/lib/sentry/configuration.rb +++ b/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] 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 diff --git a/sentry-ruby/lib/sentry/data_collection.rb b/sentry-ruby/lib/sentry/data_collection.rb index 6fcd439a7..0fac44c71 100644 --- a/sentry-ruby/lib/sentry/data_collection.rb +++ b/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) @@ -140,5 +151,11 @@ def initialize @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 diff --git a/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb b/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb index 471d9059e..14f0502d9 100644 --- a/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb +++ b/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) diff --git a/sentry-ruby/lib/sentry/event.rb b/sentry-ruby/lib/sentry/event.rb index 785df8e5b..9e5a6ae40 100644 --- a/sentry-ruby/lib/sentry/event.rb +++ b/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 diff --git a/sentry-ruby/lib/sentry/interfaces/request.rb b/sentry-ruby/lib/sentry/interfaces/request.rb index 2abe0b6e9..ad777ae8b 100644 --- a/sentry-ruby/lib/sentry/interfaces/request.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index 95d489d35..e60fc1337 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/configuration_spec.rb b/sentry-ruby/spec/sentry/configuration_spec.rb index d290e1a61..9f94bbcfd 100644 --- a/sentry-ruby/spec/sentry/configuration_spec.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb b/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb index 501927616..4b0f1ce97 100644 --- a/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/data_collection_spec.rb b/sentry-ruby/spec/sentry/data_collection_spec.rb index 99f6fe0a8..14c0969e8 100644 --- a/sentry-ruby/spec/sentry/data_collection_spec.rb +++ b/sentry-ruby/spec/sentry/data_collection_spec.rb @@ -3,6 +3,15 @@ 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 @@ -10,8 +19,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) @@ -31,16 +42,36 @@ end end + describe "#collect_incoming_http_body?" do + it "collects the body when http_bodies is nil" do + data_collection.http_bodies = nil + + expect(data_collection.collect_incoming_http_body?).to eq(true) + end + + it "collects the body when incoming requests are configured" do + data_collection.http_bodies = [:incoming_request] + + expect(data_collection.collect_incoming_http_body?).to eq(true) + end + + it "does not collect the body when incoming requests are not configured" do + data_collection.http_bodies = [:outgoing_request] + + expect(data_collection.collect_incoming_http_body?).to eq(false) + end + end + describe "defaults" do it "uses the defaults from the Data Collection specification" do expect(data_collection.user_info).to eq(true) 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) diff --git a/sentry-ruby/spec/sentry/event_spec.rb b/sentry-ruby/spec/sentry/event_spec.rb index 08a60335f..062af9826 100644 --- a/sentry-ruby/spec/sentry/event_spec.rb +++ b/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: {} ) diff --git a/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb b/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb index 57b569621..13420f265 100644 --- a/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb +++ b/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb @@ -5,10 +5,20 @@ RSpec.describe Sentry::RequestInterface do let(:env) { Rack::MockRequest.env_for("/test") } let(:send_default_pii) { false } + let(:configuration) do + Sentry::Configuration.new do |config| + config.send_default_pii = send_default_pii + end + end + let(:data_collection) { configuration.data_collection } let(:rack_env_whitelist) { Sentry::Configuration::RACK_ENV_WHITELIST_DEFAULT } subject do - described_class.new(env: env, send_default_pii: send_default_pii, rack_env_whitelist: rack_env_whitelist) + described_class.new( + env: env, + data_collection: data_collection, + rack_env_whitelist: rack_env_whitelist + ) end describe "rack_env_whitelist" do @@ -33,8 +43,8 @@ context "with empty whitelist" do let(:rack_env_whitelist) { [] } - it 'keeps the original env intact' do - expect(subject.env).to eq(env) + it 'keeps the original env values intact' do + expect(subject.env).to include(env) end end end @@ -45,7 +55,7 @@ it 'transforms headers to conform with the interface' do expect(subject.headers).to include("Version" => "HTTP/1.1", "X-Request-Id" => "12345678") - expect(subject.headers).not_to include("Cookie") + expect(subject.headers).to include("Cookie" => "[Filtered]") end context 'from Rails middleware' do @@ -80,9 +90,9 @@ end it "doesn't capture cookies info" do - env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => "cookies!") + env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => { "my" => "cookies!" }) - expect(subject.cookies).to eq(nil) + expect(subject.cookies).to eq({}) expect(subject.env["COOKIE"]).to eq(nil) end @@ -90,7 +100,7 @@ it "filters out HTTP_COOKIE header" do env.merge!("HTTP_COOKIE" => "cookies!") - expect(subject.headers["Cookie"]).to eq(nil) + expect(subject.headers["Cookie"]).to eq("[Filtered]") end it "filters out non-http headers" do @@ -118,10 +128,10 @@ expect(subject.headers).to include("Http-Custom-Http-Header" => "test") end - it "skips Authorization header" do + it "filters Authorization header" do env.merge!("HTTP_AUTHORIZATION" => "Basic YWxhZGRpbjpvcGVuc2VzYW1l") - expect(subject.headers["Authorization"]).to eq(nil) + expect(subject.headers["Authorization"]).to eq("[Filtered]") end it 'does not fail if an object in the env cannot be cast to string' do @@ -133,7 +143,13 @@ def to_s env.merge!("HTTP_FOO" => "BAR", "rails_object" => obj) - expect { described_class.new(env: env, send_default_pii: send_default_pii, rack_env_whitelist: rack_env_whitelist) }.to_not raise_error + expect do + described_class.new( + env: env, + data_collection: data_collection, + rack_env_whitelist: rack_env_whitelist + ) + end.to_not raise_error end end @@ -155,52 +171,255 @@ def to_s expect(subject.query_string).to eq(nil) end + it "doesn't fail on a malformed query string" do + env.merge!("QUERY_STRING" => "a=%") + + expect { subject }.not_to raise_error + expect(subject.query_string).to eq(nil) + end + + it "doesn't fail on query parameters with conflicting types" do + env.merge!("QUERY_STRING" => "a[]=1&a[x]=2") + + expect { subject }.not_to raise_error + expect(subject.query_string).to eq(nil) + end + + describe "data_collection" do + context "when cookies are disabled" do + before do + data_collection.cookies.mode = :off + env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => { "session" => "secret", "name" => "Ada" }) + end + + it "does not collect cookies" do + expect(subject.cookies).to eq({}) + end + end + + context "when cookies use an allow list" do + before do + data_collection.cookies.mode = :allow_list + data_collection.cookies.terms = ["name"] + env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => { "session" => "secret", "name" => "Ada" }) + end + + it "only collects allowed cookies" do + expect(subject.cookies).to eq("session" => "[Filtered]", "name" => "Ada") + end + end + + context "when cookies use a deny list" do + before do + data_collection.cookies.mode = :deny_list + data_collection.cookies.terms = ["private"] + env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => { "private_data" => "secret", "name" => "Ada" }) + end + + it "filters matching cookies and collects the others" do + expect(subject.cookies).to eq("private_data" => "[Filtered]", "name" => "Ada") + end + end + + context "when query parameters use an allow list" do + before do + data_collection.url_query_params.mode = :allow_list + data_collection.url_query_params.terms = ["page"] + env.merge!("QUERY_STRING" => "token=secret&page=2") + end + + it "only collects allowed query parameters" do + expect(subject.query_string).to eq("token" => "[Filtered]", "page" => "2") + end + end + + context "when query parameters use a deny list" do + before do + data_collection.url_query_params.mode = :deny_list + data_collection.url_query_params.terms = ["private"] + env.merge!("QUERY_STRING" => "private_data=secret&page=2") + end + + it "filters matching query parameters and collects the others" do + expect(subject.query_string).to eq("private_data" => "[Filtered]", "page" => "2") + end + end + + context "when request headers use an allow list" do + before do + data_collection.http_headers.request.mode = :allow_list + data_collection.http_headers.request.terms = ["public"] + env.merge!( + "HTTP_X_PUBLIC" => "visible", + "HTTP_X_PRIVATE" => "private", + "HTTP_AUTHORIZATION" => "secret" + ) + end + + it "only collects allowed headers and always filters sensitive values" do + expect(subject.headers).to include( + "X-Public" => "visible", + "X-Private" => "[Filtered]", + "Authorization" => "[Filtered]" + ) + end + end + + context "when request headers use a deny list" do + before do + data_collection.http_headers.request.mode = :deny_list + data_collection.http_headers.request.terms = ["private"] + env.merge!( + "HTTP_X_PUBLIC" => "visible", + "HTTP_X_PRIVATE" => "private", + "HTTP_AUTHORIZATION" => "secret" + ) + end + + it "filters matching headers, collects the others, and always filters sensitive values" do + expect(subject.headers).to include( + "X-Public" => "visible", + "X-Private" => "[Filtered]", + "Authorization" => "[Filtered]" + ) + end + end + + context "when incoming request bodies are configured" do + before do + data_collection.http_bodies = [:incoming_request] + env.merge!( + "REQUEST_METHOD" => "POST", + ::Rack::RACK_INPUT => StringIO.new("name=Ada") + ) + end + + it "collects the request body" do + expect(subject.data).to eq("name" => "Ada") + end + + it "collects and filters a JSON request body" do + env.merge!( + "CONTENT_TYPE" => "application/json", + ::Rack::RACK_INPUT => StringIO.new('{"password":"secret","name":"Ada"}') + ) + + expect(subject.data).to eq("password" => "[Filtered]", "name" => "Ada") + end + end + + context "when incoming request bodies are not configured" do + before do + data_collection.http_bodies = [:outgoing_request] + env.merge!( + "REQUEST_METHOD" => "POST", + ::Rack::RACK_INPUT => StringIO.new("name=Ada") + ) + end + + it "does not collect the request body" do + expect(subject.data).to be_nil + end + end + + context "when request data collection is disabled" do + let(:rack_env_whitelist) { [] } + + before do + data_collection.cookies.mode = :off + data_collection.http_headers.request.mode = :off + data_collection.http_bodies = [] + data_collection.url_query_params.mode = :off + env.merge!( + "QUERY_STRING" => "page=2", + "HTTP_X_PUBLIC" => "visible", + ::Rack::RACK_REQUEST_COOKIE_HASH => { "name" => "Ada" }, + ::Rack::RACK_INPUT => StringIO.new("name=Ada") + ) + end + + it "does not collect cookies, query parameters, headers, env, or body" do + expect(subject.cookies).to eq({}) + expect(subject.query_string).to be_nil + expect(subject.headers).to eq({}) + expect(subject.env).to eq({}) + expect(subject.data).to be_nil + end + end + end + context "with config.send_default_pii = true" do let(:send_default_pii) { true } it "stores cookies" do - env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => "cookies!") + env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => { "my" => "cookies!" }) - expect(subject.cookies).to eq("cookies!") + expect(subject.cookies).to eq({ "my" => "cookies!" }) end - it "stores form data" do - env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=catch me")) + it "stores and filters form data" do + env.merge!( + "REQUEST_METHOD" => "POST", + ::Rack::RACK_INPUT => StringIO.new("password=secret&name=Ada") + ) - expect(subject.data).to eq({ "data" => "catch me" }) + expect(subject.data).to eq("password" => "[Filtered]", "name" => "Ada") end - it "stores query string" do - env.merge!("QUERY_STRING" => "token=xxxx") + it "stores and filters query string values" do + env.merge!("QUERY_STRING" => "token=xxxx&page=2") - expect(subject.query_string).to eq("token=xxxx") + expect(subject.query_string).to eq("token" => "[Filtered]", "page" => "2") end - it "stores request body" do - env.merge!(::Rack::RACK_INPUT => StringIO.new("catch me")) + it "stores text request bodies" do + env.merge!("CONTENT_TYPE" => "application/text", ::Rack::RACK_INPUT => StringIO.new("catch me")) expect(subject.data).to eq("catch me") end + it "filters invalid JSON request bodies" do + env.merge!("CONTENT_TYPE" => "application/json", ::Rack::RACK_INPUT => StringIO.new("invalid")) + + expect(subject.data).to include("unexpected") + end + + it "filters sensitive values in JSON request bodies" do + env.merge!( + "CONTENT_TYPE" => "application/json", + ::Rack::RACK_INPUT => StringIO.new('{"password":"secret","name":"Ada"}') + ) + + expect(subject.data).to eq("password" => "[Filtered]", "name" => "Ada") + end + + ["null", "true", "42", '"text"', '["value"]'].each do |body| + it "stores non-object JSON request body: #{body}" do + env.merge!("CONTENT_TYPE" => "application/json", ::Rack::RACK_INPUT => StringIO.new(body)) + + expect(subject.data).to eq(JSON.parse(body)) + end + end + it "does not try to read non rewindable body" do env.merge!(::Rack::RACK_INPUT => double) expect(subject.data).to eq("Skipped non-rewindable request body") end - it "reads rewindable body" do + it "stores rewindable text bodies" do dbl = double allow(dbl).to receive(:rewind) allow(dbl).to receive(:read).and_return("stuff") - env.merge!(::Rack::RACK_INPUT => dbl) + env.merge!("CONTENT_TYPE" => "application/text", ::Rack::RACK_INPUT => dbl) expect(subject.data).to eq("stuff") end - it "stores Authorization header" do + it "Authorization header is sensitive and still filtered" do env.merge!("HTTP_AUTHORIZATION" => "Basic YWxhZGRpbjpvcGVuc2VzYW1l") - expect(subject.headers["Authorization"]).to eq("Basic YWxhZGRpbjpvcGVuc2VzYW1l") + expect(subject.headers["Authorization"]).to eq("[Filtered]") end it "force encodes request body to avoid encoding issue" do diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 8a037595b..734952339 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -267,7 +267,7 @@ def inspect { "REQUEST_METHOD" => "POST", "CONTENT_TYPE" => "application/text", ::Rack::RACK_INPUT => StringIO.new("stuff") } end - it "captures the exception with request form data" do + it "captures the exception request text body" do app = ->(_e) { raise exception } stack = Sentry::Rack::CaptureExceptions.new(app) @@ -287,7 +287,7 @@ def inspect end end - context "with non rewindable non form data" do + context "with non rewindable body" do let(:dbl) { double } let(:additional_headers) do { "REQUEST_METHOD" => "POST", "CONTENT_TYPE" => "application/text", ::Rack::RACK_INPUT => dbl }