Skip to content

always convert trusted proxies to string#1288

Merged
st0012 merged 5 commits intogetsentry:masterfrom
sortlist:trusted_proxies
Feb 16, 2021
Merged

always convert trusted proxies to string#1288
st0012 merged 5 commits intogetsentry:masterfrom
sortlist:trusted_proxies

Conversation

@gagalago
Copy link
Copy Markdown
Contributor

@gagalago gagalago commented Feb 15, 2021

fix #1274

Description

IPAddr.new only accept the address as a string but not as an IPAddr as suggested in the Rails documentation.

@gagalago
always convert trusted proxies to string
1a317c8 
fix getsentry#1274

`IPAddr.new` only accept the address as a string but not as an `IPAddr` as suggested in the Rails documentation https://api.rubyonrails.org/classes/ActionDispatch/RemoteIp.html
st0012
st0012 suggested changes Feb 15, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

thanks for the PR, but I think the solution can still be improved 🙂

although sentry-rails is where most users would register the trusted proxies, config.trusted_proxies is also a public configuration option. so even though the PR can stop most of the problematic cases, it doesn't prevent users from manually injecting the converted ip addresses. imo, it's the Utils::RealIP's responsibility to check every ip address' type and whether to convert them.

also, there should be test cases to cover the situation this PR wants to address.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 16, 2021 •
edited
Loading

Codecov Report

Merging #1288 (0ed3a3e) into master (5c4938c) will increase coverage by 0.57%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1288      +/-   ##
==========================================
+ Coverage   97.94%   98.51%   +0.57%     
==========================================
  Files         203      108      -95     
  Lines        8749     4923    -3826     
==========================================
- Hits         8569     4850    -3719     
+ Misses        180       73     -107
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/utils/real_ip.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/utils/real_ip_spec.rb 100.00% <100.00%> (ø)
...ntry-raven/lib/raven/core_ext/object/duplicable.rb
...try-raven/lib/sentry-raven-without-integrations.rb
sentry-raven/spec/raven/cli_spec.rb
sentry-raven/lib/raven/configuration.rb
sentry-raven/lib/raven/integrations/sidekiq.rb
...try-raven/lib/raven/utils/exception_cause_chain.rb
...lib/raven/integrations/rails/controller_methods.rb
sentry-raven/lib/raven/logger.rb
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c4938c...0ed3a3e. Read the comment docs.

@st0012 st0012 added this to the sentry-ruby-4.2.2 milestone Feb 16, 2021
st0012
st0012 approved these changes Feb 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

thanks for the fix! can you also add a changelog entry?

@gagalago
Copy link
Copy Markdown
Contributor Author

gagalago commented Feb 16, 2021

thanks for the fix! can you also add a changelog entry?

I did it by hand. As it's not possible to do it with craft when it's not the master branch.

Can be interesting for next time 😉 to add on the CONTRIBUTING.md or in the pull request template who is responsible to add the entry in the changelog and how to do it in a feature branch (to be compatible later with craft).

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 16, 2021

@gagalago thanks for the advice 😄 I also add changelogs by hand tbh. but I'll mention the changelog entry in the pr template 👍

@st0012 st0012 merged commit 4807ae8 into getsentry:master Feb 16, 2021
@gagalago gagalago deleted the trusted_proxies branch February 17, 2021 08:28
@dependabot dependabot bot mentioned this pull request Mar 16, 2021
Merged
@vojtad vojtad mentioned this pull request Jul 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@woidda woidda woidda approved these changes

@st0012 st0012 st0012 approved these changes

@mihaic195 mihaic195 mihaic195 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sentry-ruby

Projects

None yet

Milestone

sentry-ruby-4.2.2

Development

Successfully merging this pull request may close these issues.

Issue with 4.2.0 -> IPAddr::AddressFamilyError: address family must be specified

5 participants

@gagalago @codecov-io @st0012 @woidda @mihaic195