Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don't gate request query strings and form data behind send_default_pii #2452

Draft
wants to merge 1 commit into
base: 6.0-dev
Choose a base branch
from

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Oct 31, 2024

This makes it consistent with behavior in other SDKs.
But since it changes behavior, we will release it as part of the next major.

@sl0thentr0py sl0thentr0py force-pushed the neel/fix-rails-pii-params branch from 78c4fe5 to f1c1f1f Compare October 31, 2024 13:14
@sl0thentr0py sl0thentr0py requested a review from solnic October 31, 2024 13:15
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.06%. Comparing base (913c75e) to head (c6d6137).

Additional details and impacted files
@@             Coverage Diff             @@
##           6.0-dev    #2452      +/-   ##
===========================================
+ Coverage    97.94%   98.06%   +0.12%     
===========================================
  Files          126      126              
  Lines         4714     4708       -6     
===========================================
  Hits          4617     4617              
+ Misses          97       91       -6     
Components Coverage Δ
sentry-ruby 98.47% <100.00%> (-0.01%) ⬇️
sentry-rails 96.72% <ø> (+0.46%) ⬆️
sentry-sidekiq 97.63% <ø> (+1.89%) ⬆️
sentry-resque 91.42% <ø> (-1.43%) ⬇️
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 99.31% <ø> (ø)
Files with missing lines Coverage Δ
sentry-ruby/lib/sentry/faraday.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/interfaces/request.rb 95.08% <100.00%> (-0.08%) ⬇️
sentry-ruby/lib/sentry/net/http.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/utils/http_tracing.rb 100.00% <ø> (ø)

... and 8 files with indirect coverage changes

@sl0thentr0py sl0thentr0py removed the request for review from solnic October 31, 2024 13:18
@sl0thentr0py sl0thentr0py marked this pull request as draft October 31, 2024 13:18
@sl0thentr0py sl0thentr0py force-pushed the neel/fix-rails-pii-params branch 3 times, most recently from 55bbe4d to ca51cc7 Compare October 31, 2024 13:51
@sl0thentr0py sl0thentr0py force-pushed the neel/fix-rails-pii-params branch from ca51cc7 to c6d6137 Compare October 31, 2024 14:03
@sl0thentr0py sl0thentr0py requested a review from solnic October 31, 2024 14:07
@sl0thentr0py sl0thentr0py marked this pull request as ready for review October 31, 2024 14:07
@sl0thentr0py
Copy link
Member Author

The failing test seems to be about new behavior rails 7.2 and will be fixed separately

@sl0thentr0py sl0thentr0py requested a review from st0012 October 31, 2024 14:08
@cleptric
Copy link
Member

Query strings should not be guarded by send_default_pii, but it should still apply for the request body. https://develop.sentry.dev/sdk/expected-features/data-handling/#sensitive-data

@sl0thentr0py sl0thentr0py marked this pull request as draft October 31, 2024 14:42
Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think send_default_pii should be applied to body.

@@ -20,7 +20,7 @@ def record_sentry_breadcrumb(request_info, response_status)
level: :info,
category: self.class::BREADCRUMB_CATEGORY,
type: :info,
data: { status: response_status, **request_info }
data: { status: response_status, **request_info.compact }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this should be called at the caller's extract_request_info helpers.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants