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

Rails controller span data is slightly modified to comply with URL
collection spec.

Author: sl0thentr0py

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

@@ -1,10 +1,14 @@
 # frozen_string_literal: true
 
+require "sentry/utils/http_tracing"
+
 module Sentry
   module Rails
     module ControllerTransaction
       SPAN_ORIGIN = "auto.view.rails"
 
+      include Sentry::Utils::HttpTracing
+
       def self.included(base)
         base.prepend_around_action(:sentry_around_action)
       end
@@ -24,9 +28,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}?#{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/railtie.rb

@@ -48,6 +48,7 @@ class Railtie < ::Rails::Railtie
       next unless Sentry.initialized?
 
       configure_project_root
+      configure_data_collection
       configure_trusted_proxies
       configure_cron_timezone
       extend_controller_methods if defined?(ActionController)
@@ -82,6 +83,14 @@ def configure_project_root
       Sentry.configuration.project_root = ::Rails.root.to_s
     end
 
+    def configure_data_collection
+      collection = Sentry.configuration.data_collection.url_query_params
+      return unless collection.mode == :deny_list
+
+      filter_parameters = ::Rails.application.config.filter_parameters
+      collection.terms = Array(collection.terms) + Array(filter_parameters)
+    end
+
     def configure_trusted_proxies
       Sentry.configuration.trusted_proxies += Array(::Rails.application.config.action_dispatch.trusted_proxies)
     end

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/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,21 @@
     end
   end
 
+  context "data collection" do
+    before do
+      make_basic_app do |config|
+        config.data_collection.url_query_params.mode = :deny_list
+      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
+  end
+
   context "at exit" do
     before do
       make_basic_app