feat(data-collection): Port Rails controller and ActiveStorage (#3033)

* Rails controller span data query inclusion is modified to comply with URL collection spec.
* Rails `filter_parameters` are backfilled into `url_query_params`
  * Allow regexes in `terms` as a result to enable this

Author: sl0thentr0py

sentry-rails/lib/sentry/rails/configuration.rb

@@ -36,6 +36,14 @@ class Configuration
 
     after(:configured) do
       rails.structured_logging.enabled = enable_logs if rails.structured_logging.enabled.nil?
+
+      collection = data_collection.url_query_params
+      if collection.mode == :deny_list
+        filter_parameters = ::Rails.application.config.filter_parameters.select do |filter|
+          filter.is_a?(String) || filter.is_a?(Symbol) || filter.is_a?(Regexp)
+        end
+        collection.terms = (Array(collection.terms) + filter_parameters).uniq
+      end
     end
   end
 

sentry-rails/lib/sentry/rails/controller_transaction.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "sentry/utils/http_tracing"
+
 module Sentry
   module Rails
     module ControllerTransaction
@@ -24,9 +26,16 @@ def sentry_around_action
                 child_span.set_data(:format, request.format)
                 child_span.set_data(:method, request.method)
 
-                pii = Sentry.configuration.send_default_pii
-                child_span.set_data(:path, pii ? request.fullpath : request.filtered_path)
-                child_span.set_data(:params, pii ? request.params : request.filtered_parameters)
+                data_collection = Sentry.configuration.data_collection
+                query = data_collection.url_query_params.filter(request.query_parameters)
+                path = request.path
+                path = "#{path}?#{Sentry::Utils::HttpTracing.format_query(query)}" unless query.empty?
+
+                child_span.set_data(:path, path)
+                child_span.set_data(
+                  :params,
+                  data_collection.url_query_params.filter(request.params)
+                )
               end
 
               result

sentry-rails/lib/sentry/rails/tracing/active_storage_subscriber.rb

@@ -34,7 +34,10 @@ def self.subscribe!
             ) do |span|
               payload.each do |key, value|
                 next if key == START_TIMESTAMP_NAME
-                next if key == :key && !Sentry.configuration.send_default_pii
+                # Active Storage keys are automatically generated storage data,
+                # rather than URL/query data. Use the database query-data switch
+                # for the legacy PII-compatible on/off behavior.
+                next if key == :key && !Sentry.configuration.data_collection.database_query_data
 
                 span.set_data(key, value)
               end

sentry-rails/spec/dummy/test_rails_app/config/application.rb

@@ -109,8 +109,10 @@ def configure
             :custom_secret,
             :api_key,
             :credit_card,
+            /billing_reference/,
             :authorization,
-            :token
+            :token,
+            proc { |_key, _value| }
           ]
 
           # Eager load namespaces can be accumulated after repeated initializations and make initialization

sentry-rails/spec/sentry/rails/tracing/active_storage_subscriber_spec.rb

@@ -79,6 +79,54 @@
     end
   end
 
+  context "data collection" do
+    context "with database query data enabled" do
+      before do
+        make_basic_app do |config|
+          config.traces_sample_rate = 1.0
+          config.rails.tracing_subscribers = [described_class]
+          config.data_collection.database_query_data = true
+        end
+      end
+
+      it "records the :key in span.data" do
+        ActiveStorage::AnalyzeJob.queue_adapter.perform_enqueued_jobs = true
+
+        p = Post.create!
+        get "/posts/#{p.id}/attach"
+
+        request_transaction = transport.events.last.to_h
+        upload_span = request_transaction[:spans].find { |s| s[:op] == "file.service_upload.active_storage" }
+
+        expect(upload_span).not_to be_nil
+        expect(upload_span.dig(:data, :key)).to eq(p.cover.key)
+      end
+    end
+
+    context "with database query data disabled" do
+      before do
+        make_basic_app do |config|
+          config.traces_sample_rate = 1.0
+          config.rails.tracing_subscribers = [described_class]
+          config.data_collection.database_query_data = false
+        end
+      end
+
+      it "does not record the :key in span.data" do
+        ActiveStorage::AnalyzeJob.queue_adapter.perform_enqueued_jobs = true
+
+        p = Post.create!
+        get "/posts/#{p.id}/attach"
+
+        request_transaction = transport.events.last.to_h
+        upload_span = request_transaction[:spans].find { |s| s[:op] == "file.service_upload.active_storage" }
+
+        expect(upload_span).not_to be_nil
+        expect(upload_span.dig(:data, :key)).to be_nil
+      end
+    end
+  end
+
   context "when transaction is not sampled" do
     before do
       make_basic_app

sentry-rails/spec/sentry/rails/tracing_spec.rb

@@ -201,17 +201,15 @@
         end
       end
 
-      it "does not record sensitive params" do
+      it "does not record any params" do
         get "/posts?foo=bar&password=42&secret=baz"
         transaction = transport.events.last.to_h
 
         params = transaction[:spans][0][:data][:params]
-        expect(params["foo"]).to eq("bar")
-        expect(params["password"]).to eq("[FILTERED]")
-        expect(params["secret"]).to eq("[FILTERED]")
+        expect(params).to be_empty
 
         path = transaction[:spans][0][:data][:path]
-        expect(path).to eq("/posts?foo=bar&password=[FILTERED]&secret=[FILTERED]")
+        expect(path).to eq("/posts")
       end
     end
 
@@ -223,17 +221,61 @@
         end
       end
 
-      it "records all params" do
+      it "records all params except sensitive params" do
         get "/posts?foo=bar&password=42&secret=baz"
         transaction = transport.events.last.to_h
 
         params = transaction[:spans][0][:data][:params]
         expect(params["foo"]).to eq("bar")
-        expect(params["password"]).to eq("42")
-        expect(params["secret"]).to eq("baz")
+        expect(params["password"]).to eq("[Filtered]")
+        expect(params["secret"]).to eq("[Filtered]")
+
+        path = transaction[:spans][0][:data][:path]
+        expect(path).to eq("/posts?foo=bar&password=[Filtered]&secret=[Filtered]")
+      end
+    end
+  end
+
+  context "data collection" do
+    context "with url query params enabled" do
+      before do
+        make_basic_app do |config|
+          config.traces_sample_rate = 1.0
+          config.data_collection.url_query_params.mode = :deny_list
+        end
+      end
+
+      it "records query params except sensitive params" do
+        get "/posts?foo=bar&password=42&secret=baz"
+        transaction = transport.events.last.to_h
+
+        params = transaction[:spans][0][:data][:params]
+        expect(params["foo"]).to eq("bar")
+        expect(params["password"]).to eq("[Filtered]")
+        expect(params["secret"]).to eq("[Filtered]")
+
+        path = transaction[:spans][0][:data][:path]
+        expect(path).to eq("/posts?foo=bar&password=[Filtered]&secret=[Filtered]")
+      end
+    end
+
+    context "with url query params disabled" do
+      before do
+        make_basic_app do |config|
+          config.traces_sample_rate = 1.0
+          config.data_collection.url_query_params.mode = :off
+        end
+      end
+
+      it "does not record query params" do
+        get "/posts?foo=bar&password=42&secret=baz"
+        transaction = transport.events.last.to_h
+
+        params = transaction[:spans][0][:data][:params]
+        expect(params).to be_empty
 
         path = transaction[:spans][0][:data][:path]
-        expect(path).to eq("/posts?foo=bar&password=42&secret=baz")
+        expect(path).to eq("/posts")
       end
     end
   end

sentry-rails/spec/sentry/rails_spec.rb

@@ -105,6 +105,38 @@
     end
   end
 
+  context "data collection" do
+    before do
+      make_basic_app do |config|
+        config.data_collection.url_query_params.mode = :deny_list
+        Rails.application.config.filter_parameters << proc { |_key, _value| }
+      end
+    end
+
+    it "adds Rails filter parameters to URL query parameter data collection" do
+      expect(Sentry.configuration.data_collection.url_query_params.terms).to include(
+        "password",
+        "custom_secret"
+      )
+    end
+
+    it "preserves and applies Regexp Rails filter parameters" do
+      expect(Sentry.configuration.data_collection.url_query_params.terms).to include(/billing_reference/)
+      expect(Sentry.configuration.data_collection.url_query_params.filter(
+        { "billing_reference_id" => "secret", "public" => "visible" }
+      )).to eq(
+        "billing_reference_id" => "[Filtered]",
+        "public" => "visible"
+      )
+    end
+
+    it "only adds String, Symbol, and Regexp Rails filter parameters" do
+      terms = Sentry.configuration.data_collection.url_query_params.terms
+
+      expect(terms).to all(satisfy { |term| term.is_a?(String) || term.is_a?(Symbol) || term.is_a?(Regexp) })
+    end
+  end
+
   context "at exit" do
     before do
       make_basic_app

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

@@ -61,7 +61,7 @@ class KeyValueCollection
       attr_accessor :mode
 
       # `terms` contains the keys or patterns used by the selected mode.
-      # @return [Array<String>, nil]
+      # @return [Array<String, Regexp>, nil]
       attr_reader :terms
 
       def initialize(mode:, terms:)
@@ -70,7 +70,8 @@ def initialize(mode:, terms:)
       end
 
       def terms=(terms)
-        @terms = terms&.map { |term| term.to_s.downcase }&.reject { |term| term.strip.empty? }
+        @terms = terms&.map { |term| term.is_a?(Regexp) ? term : term.to_s.downcase }
+          &.reject { |term| !term.is_a?(Regexp) && term.strip.empty? }
       end
 
       # Applies this collection configuration without changing the input hash.
@@ -90,14 +91,15 @@ def filter(values, cookie: false)
       private
 
       def safe_value?(key, cookie: false)
-        key_downcase = key.to_s.downcase
+        key_string = key.to_s
+        key_downcase = key_string.downcase
         return false if sensitive?(key_downcase, cookie: cookie)
 
         case mode
         when :deny_list
-          !matches_any_term?(key_downcase)
+          !matches_any_term?(key_string, key_downcase)
         when :allow_list
-          matches_any_term?(key_downcase)
+          matches_any_term?(key_string, key_downcase)
         else
           false
         end
@@ -108,8 +110,10 @@ def sensitive?(key, cookie: false)
           (cookie && SENSITIVE_COOKIE_NAME_DENY_LIST.any? { |term| key.include?(term) })
       end
 
-      def matches_any_term?(key)
-        @terms&.any? { |term| key.include?(term) }
+      def matches_any_term?(key, key_downcase)
+        @terms&.any? do |term|
+          term.is_a?(Regexp) ? term.match?(key) : key_downcase.include?(term)
+        end
       end
     end
   end

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

@@ -20,12 +20,12 @@ def filter_query_params(query)
         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)
+        HttpTracing.format_query(filtered_query_hash)
       rescue
         nil
       end
 
-      def format_query(query)
+      def self.format_query(query)
         query.flat_map do |key, value|
           Array(value).map { |item| "#{key}=#{item}" }
         end.join("&")

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

@@ -54,6 +54,23 @@
         )
       end
 
+      it "matches regular expression terms" do
+        expect(described_class.new(mode: :deny_list, terms: [/private[-_]data/]).filter(
+          { "private_data" => "secret", "PRIVATE_DATA" => "visible", "public_data" => "visible" }
+        )).to eq(
+          "private_data" => "[Filtered]",
+          "PRIVATE_DATA" => "visible",
+          "public_data" => "visible"
+        )
+      end
+
+      it "preserves regular expression terms when assigning terms" do
+        regexp = /private/i
+        collection.terms = [regexp]
+
+        expect(collection.terms).to eq([regexp])
+      end
+
       it "normalizes terms assigned after initialization" do
         collection.terms = ["USER"]