Skip to content

Commit

Permalink
Fix send_default_pii handling in rails controller spans (#2443)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sl0thentr0py authored Oct 22, 2024
1 parent 1c431f0 commit 9bba2ef
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 3 deletions.
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 4 additions & 2 deletions sentry-rails/lib/sentry/rails/controller_transaction.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions sentry-rails/spec/dummy/test_rails_app/app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 46 additions & 0 deletions sentry-rails/spec/sentry/rails/tracing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down

0 comments on commit 9bba2ef

Please sign in to comment.