Use last forwarded IP #68884
Merged
Use last forwarded IP #68884
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Well-behaved forwarders will append the IP they're forwarding for to an existing list. In the most typical case, this means that only the last one is trustworthy from a spoofing request-maker. And all this is assuming that the proxy itself is trusted. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#security_and_privacy_concerns
github-actions
bot
added
the
Scope: Backend
JoshFerge
reviewed
Apr 16, 2024
Looks like there was already a test of the old behavior. I updated it to reflect the new behavior. |
JoshFerge
added
the
Trigger: getsentry tests
|
fwiw, this is general good practice for use in front of load balancers, but I don't know nearly enough about the many ways that Sentry is used to know whether this is an important attack vector. I'm reading some comments about using |
Yes I'm talking with some SRE folks internally about some of the security details of this -- been a long time since i ran a server :) hope to make progress on this soon. |
|
I'm currently having to put my Sentry tunnel behind an Apache proxy which is behind an ALB. And I can't figure out how to get Apache to not mess with the X-Forwarded-* headers. So for the risks of spoofing, getting the first from the list is actually probably better for me at the moment, so that I don't have to rewrite those headers in my tunnel, I can just pass them along. |
oioki
approved these changes
May 6, 2024
iamrajjoshi
added a commit
that referenced
this pull request
May 8, 2024
commit 1354478 Author: Raj Joshi <raj.joshi@sentry.io> Date: Wed May 8 09:56:24 2024 -0700 feat(chartcuterie): Change Chart Stylings (#70489) two quick updates: 1. Changes the label for regressed to a darker color so we can read it better  2. Updated some chart styling for slack so the legend is on the left  commit 56514bc Author: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Wed May 8 12:47:19 2024 -0400 ref(replay): Rage click clicked element name (#70493) Since we now use react component names in the selector path, we should modify clicked element to provide more specific info such as class, role etc. This prevents giving the react component name twice and would give more info for debugging since react component names aren't very specific Before: <img width="845" alt="image" src="https://github.com/getsentry/sentry/assets/55311782/87b6c8cd-1b29-40bb-b448-715595cb255b"> commit 35b38d9 Author: Steven Eubank <47563310+smeubank@users.noreply.github.com> Date: Wed May 8 18:38:57 2024 +0200 Add Deno Runtime Icon (#69100) QoL, this has been bugging me Should show the Deno icon, from Deno runtimes hosted on Supabase and others after:   commit a96f1ff Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Wed May 8 12:34:37 2024 -0400 ref(crons): Improve details legend (#70515) The legend now differentiates between timeout and failed <img alt="clipboard.png" width="267" src="https://i.imgur.com/yQAfXaB.png" /> --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com> commit c3b9fdb Author: Matt Duncan <14761+mrduncan@users.noreply.github.com> Date: Wed May 8 09:23:28 2024 -0700 chore(issues): Enable stronger typing on two endpoints (#70488) This is another quick follow up to #69828 since these two type errors are trivial to resolve. commit dbc926a Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Wed May 8 12:22:14 2024 -0400 ref(crons): Move timezone to schedule text (#70511) <img width="295" alt="image" src="https://github.com/getsentry/sentry/assets/1421724/fd919e5b-0dd6-4b41-8e84-d3b99dfe6042"> It is now next to the schedule. This is more logical commit 9a90b57 Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Wed May 8 12:18:49 2024 -0400 ref(crons): Use constant for DEFAULT_CHECKIN_MARGIN (#70510) commit f161ebb Author: Colleen O'Rourke <colleen@sentry.io> Date: Wed May 8 09:16:58 2024 -0700 ref(daily summary): Disable notification (#70295) I don't currently have any time to dedicate to bug fixing and it's been sending multiple times, so this is going to be disabled for now. We'll likely have to keep track of which users per org received the notification and check that before sending, and then clear it out every hour to avoid duplicate sending. commit 7d1cf85 Author: Matt Duncan <14761+mrduncan@users.noreply.github.com> Date: Wed May 8 08:58:45 2024 -0700 chore(issues): Enable stronger typing on occurrence_consumer (#70487) This is a quick follow up to #69828 since these two type errors are trivial to resolve. commit 6b530ff Author: colin-sentry <161344340+colin-sentry@users.noreply.github.com> Date: Wed May 8 11:50:48 2024 -0400 chore(ai-monitoring): Add a unit to total cost (#70484) commit 50b4ed3 Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Wed May 8 11:32:28 2024 -0400 ref(routes): A few more routes using withOrgPath (#70449) commit 4d124bd Author: Evan Hicks <evanh@users.noreply.github.com> Date: Wed May 8 11:19:07 2024 -0400 fix: Add a metric for non-success Snuba requests (#70452) Tracking this on the Sentry side allows alerts to be created that are separate from the Snuba API itself, in case the API is in a broken state and can't accurately report what is happening. commit 11a80a3 Author: Dominik Buszowiecki <44422760+DominikB2014@users.noreply.github.com> Date: Wed May 8 10:36:32 2024 -0400 feat(cache): add average transaction duration to sample sidebar (#70445) New metric readout for avg transaction duration in sample sidebar <img width="746" alt="image" src="https://github.com/getsentry/sentry/assets/44422760/35146610-5c41-4e12-917e-e58310da84a9"> commit eb4de59 Author: Ash <0Calories@users.noreply.github.com> Date: Wed May 8 10:25:30 2024 -0400 feat(perf): Add backend referrers for span summary and span metrics (#70466) Adds backend referrers to be used on the new span summary and span metrics pages Relevant PR: #69159 commit d3d6bdf Author: Mark Story <mark@mark-story.com> Date: Wed May 8 10:20:37 2024 -0400 chore(actor) Remove Actor model from django state (#70439) Remove the Actor model from django state. Refs HC-1183 commit 5bff933 Author: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Wed May 8 08:34:04 2024 -0400 ref: remove unused partition parameter from buffer (#70441) <!-- Describe your PR here. --> commit 7512ed6 Author: Yagiz Nizipli <yagiz@nizipli.com> Date: Wed May 8 07:29:26 2024 -0400 perf: use orjson in all middlewares (#70456) commit 4623b5d Author: Tony Xiao <txiao@sentry.io> Date: Wed May 8 06:07:45 2024 -0400 fix(trace-explorer): Date range narrowing condition is backwards (#70496) This was changing the end timestamp to be too narrow and missing some spans. commit fc8b666 Author: Ryan Hiebert <ryan@ryanhiebert.com> Date: Wed May 8 02:33:09 2024 -0500 Use last forwarded IP (#68884) Well-behaved forwarders will append the IP they're forwarding for to an existing list. In the most typical case, this means that only the last one is trustworthy from a spoofing request-maker. And all this is assuming that the proxy itself is trusted. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#security_and_privacy_concerns <!-- Describe your PR here. --> <!-- Sentry employees and contractors can delete or ignore the following. --> ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. commit 01160aa Author: Katie Byers <katie.byers@sentry.io> Date: Tue May 7 16:41:32 2024 -0700 ref(seer-grouping): Switch to using `hash` and `parent_hash` from `group_hash` and `parent_group_hash` (#70383) As we've been thinking about the switch from sending and receiving group ids when communicating with Seer to doing so with hashes, all along we've been talking about those hashes as "group hashes." In truth, though, hash values are based on the data in a particular event (not the group overall), and indeed, that's how we're using them in Seer - pairing up hashes not with what group they're in but which event data they represent. There _is_ a pairing of groups and hashes - on the Sentry side, in the form of the `GroupHash` table - but entries from that table aren't what we're using with Seer. With Seer, we only care about the "hash" part of `GroupHash`. So, both for accuracy and so as to be able to differentiate in Seer-related Sentry code between hashes (hex strings) and grouphashes (association table records), we're switching from using `group_hash` and `parent_group_hash` to using `hash` and `parent_hash`. This PR makes the change on the Sentry side. Fortunately, nothing in Seer is yet relying on hashes, so as long as we wait for this to go live, we can then add hash support on the seer side using the new names from the get-go. commit 173b690 Author: Raj Joshi <raj.joshi@sentry.io> Date: Tue May 7 16:32:23 2024 -0700 fix(chartcuterie): Added Visual Map Field for Endpoint Regression (#70477) There is mismatch in the way we build the EChart Options object in our FE code and how Chartcuterie handles it. In our FE [code,](https://github.com/getsentry/sentry/blob/master/static/app/components/events/eventStatisticalDetector/breakpointChartOptions.tsx#L89-L104), we wrap the visualMap object in an extra option, which allows us to maintain the hierarchy for styling. However, Chartcuterie cannot handle the wrapped object, so when we pass the service the options, we unwrap it. I also created a modifier option to modify chart options specifically for slack and removed the icon from the legend icon from there.  commit e624ee9 Author: Matt Duncan <14761+mrduncan@users.noreply.github.com> Date: Tue May 7 16:17:45 2024 -0700 chore(issues): Opt in already passing issues files to stronger typing (#69828) `sentry.issues.*` and `test.sentry.issues.*` are not close to passing but in the mean time we can get incremental benefits and prevent regressions by opting in modules which are already passing. #69374 includes a bit more details and outlines additional fixes we can follow up this change with. commit dbf524c Author: Seiji Chew <67301797+schew2381@users.noreply.github.com> Date: Tue May 7 15:58:22 2024 -0700 fix(ui): Revert to using project release commmit API (#70485) In #63860 we switched from a class component to a FC and also switched from hitting `ProjectReleaseCommitsEndpoint` to `OrganizationReleaseCommitsEndpoint`. The latter doesn't respect repo name or id in the query param, which makes it so selecting a repo in the dropdown will show you commits from other repos (and also messes up pagination as well, not showing you all commits). Related to #70411 commit 0b96de4 Author: Dan Fuller <dfuller@sentry.io> Date: Tue May 7 15:53:42 2024 -0700 chore(crons): Rename badly named api file (#70480) I accidentally pluralised this when creating it, just fixing. commit 2489a4a Author: Dominik Buszowiecki <44422760+DominikB2014@users.noreply.github.com> Date: Tue May 7 18:19:21 2024 -0400 fix(cache): update docs link (#70475) The link to the docs will be plural commit 7ea571e Author: Katie Byers <katie.byers@sentry.io> Date: Tue May 7 15:17:08 2024 -0700 chore(events): Move `PLACEHOLDER_EVENT_TITLES` to a neutral location (#70470) I tried to use `PLACEHOLDER_EVENT_TITLES` in an upcoming PR, and landed in a circular dependency with `event_manager.py`, where it currently lives. This moves it to `constants.py`, which solves the problem. commit a97dccf Author: Michael Sun <55160142+MichaelSun48@users.noreply.github.com> Date: Tue May 7 18:11:47 2024 -0400 chore(issue-stream): Register feature flag for upcoming changes to issues stream events graph (#70471) [Project details](#69691) * [SOA PR ](getsentry/sentry-options-automator#1378) * ~~getSentry PR~~ (No longer necessary) commit fadd0b5 Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Tue May 7 18:00:45 2024 -0400 fix(ui): Use new illustration for tracing keyboard shortcuts (#70474) <img alt="clipboard.png" width="665" src="https://i.imgur.com/h8p3E1H.png" /> Looks like the rest of the product now commit 277f026 Author: Scott Cooper <scttcper@gmail.com> Date: Tue May 7 14:54:57 2024 -0700 feat(metrics): Add metricSecond to allowed category (#70442) commit ef6c79a Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Tue May 7 17:50:52 2024 -0400 Revert "ref(crons): Normalize crons incident issues (#70289)" (#70469) This reverts commit 9d56889. commit 45d2d2e Author: Cathy Teng <70817427+cathteng@users.noreply.github.com> Date: Tue May 7 14:46:55 2024 -0700 feat(slack): EA :white_circle: for actions by adding to issue alert threads FF (#70468) commit 80ed536 Author: Tony Xiao <txiao@sentry.io> Date: Tue May 7 17:25:18 2024 -0400 fix(trace-explorer): Breakdown by project and sdk (#70463) For full stack projects that use a single project for all the data, we need to break it down further using sdk. commit 874db7e Author: Katie Byers <katie.byers@sentry.io> Date: Tue May 7 14:24:19 2024 -0700 ref(seer-grouping): Send group hash to Seer (#70244) This adds the group hash to the outgoing payload sent to Seer. Once this is merged, it will be safe to remove the Seer logic handling an incoming group id, since we'll now be able to rely on the group hash being present. Taken together, this PR and the ones leading up to it (links below) mean will mean that on the Sentry side we'll be sending in the request and handling in the response both hash and group id (and transforming hashes in the response into group ids if hashes are all that's sent). Once Seer is adjusted to only deal in hashes, we can then remove the sending and handling of group ids from Sentry and the transition will be complete. Other PRs which are part of the group-id-to-group-hash switch: - #70005, #70236, and #70237 - various small fixes and tweaks - #70070 and #70238 - updates to associated types - #70240 - automatic conversion from hash to group id when handling Seer similar group data commit 78d2f28 Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Tue May 7 17:23:38 2024 -0400 ref(routes): Prioritize customer domains route (#70451) In a936fb0 we introduced a new withOrgPath prop to the Route component. This caused the route to have the same behaviour as doing ```tsx <Framgnet> {USING_CUSTOMER_DOMAIN && ( <Route path="/some-path/" component={withDomainRequired(make(() => import('sentry/views/someView')))} /> )} <Route path="/organizations/:orgId/some-path/" component={withDomainRedirect(make(() => import('sentry/views/someView')))} /> </Framgnet> ``` However the introduced logic generated the routes in the opposite order, where the org slug version would get priority. For some future changes this becomes important so I am bringing back this matching behaviour commit 3a7e9b3 Author: Evan Purkhiser <evanpurkhiser@gmail.com> Date: Tue May 7 17:23:31 2024 -0400 fix(crons): Teams -> Owners (#70464) commit 2dbccba Author: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Tue May 7 17:13:26 2024 -0400 ref: upgrade djangorestframework-stubs (#70461) upgrading this has no differences in ignored mypy errors <!-- Describe your PR here. --> commit 0c8f916 Author: Mark Story <mark@mark-story.com> Date: Tue May 7 16:49:43 2024 -0400 chore(database) Drop tables for project and team avatar (#68616) These tables no longer have django models, and can be deleted. commit 4f71b8e Author: Leander Rodrigues <leander.rodrigues@sentry.io> Date: Tue May 7 16:39:00 2024 -0400 feat(highlights): Variety of fixes/changes to highlights work (#70355) This PR makes a variety of changes to the highlights/tags areas: - [x] Add tests for analytics and user friendly names - [x] Add search to the highlights modal  - [x] Allow `replayId`, `transaction` and tags with URLs to be clickable  - [x] Adds a feedback button on Highlight section  **todo** - [x] Add tests for search - [x] Add tests for new tag links - [x] Add screenshots of changes commit aa40406 Author: Snigdha Sharma <snigdha.sharma@sentry.io> Date: Tue May 7 13:36:54 2024 -0700 fix(escalating-issues): Cleanup query for escalating forecasts (#70443) Cleans up some cruft left behind by feature flags and pushes the escalation check out of the query to help with timeouts. Fixes SENTRY-18AS commit 81d3fef Author: Seiji Chew <67301797+schew2381@users.noreply.github.com> Date: Tue May 7 13:32:10 2024 -0700 nit: Remove authenticators from state (#70365) Remove `authenticators` from state because we only set it once
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Well-behaved forwarders will append the IP they're forwarding for to an existing list. In the most typical case, this means that only the last one is trustworthy from a spoofing request-maker.
And all this is assuming that the proxy itself is trusted.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#security_and_privacy_concerns
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.