From 9bba2ef0a8ef14fa6df8a8a1e5f61e5589375b16 Mon Sep 17 00:00:00 2001
From: Neel Shah <neel.shah@sentry.io>
Date: Tue, 22 Oct 2024 17:27:10 +0200
Subject: [PATCH] Fix send_default_pii handling in rails controller spans
 (#2443)

The change in #1793 accidentally exposed params always in the controller action span.
This change now respects both `Rails.config.filter_parameters` and `Sentry.coniguration.send_default_pii`
together to decide what to send.
---
 CHANGELOG.md                                  |  5 +-
 .../sentry/rails/controller_transaction.rb    |  6 ++-
 sentry-rails/spec/dummy/test_rails_app/app.rb |  1 +
 .../spec/sentry/rails/tracing_spec.rb         | 46 +++++++++++++++++++
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b8b49da70..a9dc18876 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,12 +3,15 @@
 ### Features
 
 - Add `include_sentry_event` matcher for RSpec [#2424](https://github.com/getsentry/sentry-ruby/pull/2424)
-- Add support for Sentry Cache instrumentation, when using Rails.cache ([#2380](https://github.com/getsentry/sentry-ruby/pull/2380)) (MemoryStore and FileStore require Rails 8.0+)
+- Add support for Sentry Cache instrumentation, when using Rails.cache [#2380](https://github.com/getsentry/sentry-ruby/pull/2380)
 
+    Note: MemoryStore and FileStore require Rails 8.0+
 
 ### Bug Fixes
 
 - Fix Vernier profiler not stopping when already stopped [#2429](https://github.com/getsentry/sentry-ruby/pull/2429)
+- Fix `send_default_pii` handling in rails controller spans [#2443](https://github.com/getsentry/sentry-ruby/pull/2443)
+  - Fixes [#2438](https://github.com/getsentry/sentry-ruby/issues/2438)
 
 ## 5.21.0
 
diff --git a/sentry-rails/lib/sentry/rails/controller_transaction.rb b/sentry-rails/lib/sentry/rails/controller_transaction.rb
index 8f19bb63d..a753f06d7 100644
--- a/sentry-rails/lib/sentry/rails/controller_transaction.rb
+++ b/sentry-rails/lib/sentry/rails/controller_transaction.rb
@@ -23,8 +23,10 @@ def sentry_around_action
                 child_span.set_http_status(response.status)
                 child_span.set_data(:format, request.format)
                 child_span.set_data(:method, request.method)
-                child_span.set_data(:path, request.path)
-                child_span.set_data(:params, request.params)
+
+                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)
               end
 
               result
diff --git a/sentry-rails/spec/dummy/test_rails_app/app.rb b/sentry-rails/spec/dummy/test_rails_app/app.rb
index 4a807f587..5b1bd82a9 100644
--- a/sentry-rails/spec/dummy/test_rails_app/app.rb
+++ b/sentry-rails/spec/dummy/test_rails_app/app.rb
@@ -60,6 +60,7 @@ def self.name
   app.config.active_job.queue_adapter = :test
   app.config.cache_store = :memory_store
   app.config.action_controller.perform_caching = true
+  app.config.filter_parameters += [:password, :secret]
 
   # Eager load namespaces can be accumulated after repeated initializations and make initialization
   # slower after each run
diff --git a/sentry-rails/spec/sentry/rails/tracing_spec.rb b/sentry-rails/spec/sentry/rails/tracing_spec.rb
index 927e11c8e..36063f39d 100644
--- a/sentry-rails/spec/sentry/rails/tracing_spec.rb
+++ b/sentry-rails/spec/sentry/rails/tracing_spec.rb
@@ -106,6 +106,52 @@
     end
   end
 
+  describe "filtering pii data" do
+    context "with send_default_pii = false" do
+      before do
+        make_basic_app do |config|
+          config.traces_sample_rate = 1.0
+          config.send_default_pii = false
+        end
+      end
+
+      it "does not record sensitive params" do
+        get "/posts?foo=bar&password=42&secret=baz"
+        transaction = transport.events.last.to_hash
+
+        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 send_default_pii = true" do
+      before do
+        make_basic_app do |config|
+          config.traces_sample_rate = 1.0
+          config.send_default_pii = true
+        end
+      end
+
+      it "records all params" do
+        get "/posts?foo=bar&password=42&secret=baz"
+        transaction = transport.events.last.to_hash
+
+        params = transaction[:spans][0][:data][:params]
+        expect(params["foo"]).to eq("bar")
+        expect(params["password"]).to eq("42")
+        expect(params["secret"]).to eq("baz")
+
+        path = transaction[:spans][0][:data][:path]
+        expect(path).to eq("/posts?foo=bar&password=42&secret=baz")
+      end
+    end
+  end
+
   context "with instrumenter :otel" do
     before do
       make_basic_app do |config|