sentry/remix appears to be incorrectly identifying users based on IP address headers

[alexblack]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using? If you use the CDN bundles, please specify the exact bundle (e.g. bundle.tracing.min.js) in your SDK setup.

@sentry/remix

SDK Version

7.38.0

Framework Version

Remix 1.13.0

Link to Sentry event

https://syncwith.sentry.io/issues/3955446273/events/fef107a39ec64e20ad358cf5f26396d1/?project=5880196

SDK Setup

client:

  init({
    beforeSend,
    dsn: SENTRY_ADDON_DSN,
    tracesSampleRate: getApp() === 'syncwith' ? 1 : 0.05,
    sendDefaultPii: true,
    normalizeDepth: 6, // stringify deeper objects
    integrations: [
      new BrowserTracing({
        routingInstrumentation: remixRouterInstrumentation(
          useEffect,
          useLocation,
          useMatches
        ),
      }),
    ],
  });

server:

  init({
  beforeSend,
  sendDefaultPii: true,
  dsn: SENTRY_ADDON_DSN,
  tracesSampleRate: getApp() === 'syncwith' ? 1 : 0.05,
  normalizeDepth: 6, // stringify deeper objects
  integrations: [
    // enable HTTP calls tracing
    new Integrations.Http({ tracing: true }),
    new Integrations.RequestData({
      include: {
        ip: true,
      },
    }),
  ],
});

Steps to Reproduce

We're using a cloudflare worker in front of our website, and cloudflare cdn, and in this request:

https://syncwith.sentry.io/issues/3955446273/events/fef107a39ec64e20ad358cf5f26396d1/?project=5880196

It appears that the user was identified using IP address 141.101.69.35, which is a cloudflare IP. The correct IP to identify the user with is 2a01:cb19:8350:ed00:d0dd:fa5b:de31:8be5 found in header Cf-Connecting-Ip.

I wonder if the bug is that sentry should let Cf-Connecting-Ip take precendence over X-Forwarded-For, or, if its not properly parsing X-Forwarded-For and extracting the relevant IP (the first, not the last, in this case)

Expected Result

The user should be identified by the IP address found in cf-connecting-ip, or maybe first in x-forwarded-for

Actual Result

The user was identified by the wrong IP

[Lms24]

Hi @alexblack, thanks for writing in! Can you confirm that this issue only concerns server-side errors? (we're handling IP addresses differently for browser and server SDKs, which is the reason I'm asking)

The reason why this is happening is that we're not looking at headers for determining the ip address. In fact, we take the IP we receive from the node request:

sentry-javascript/packages/node/src/requestdata.ts

Lines 285 to 296 in a88a8bf

	// client ip:
	// node, nextjs: req.socket.remoteAddress
	// express, koa: req.ip
	if (include.ip) {
	const ip = req.ip || (req.socket && req.socket.remoteAddress);
	if (ip) {
	event.user = {
	...event.user,
	ip_address: ip,
	};
	}
	}

I'm not sure if we're going to start analyzing headers as well, given that there's probably quite a few headers out there we'd potentially need to check. I'm gonna raise this internally to see if and how we'll make changes here.

[alexblack]

Hi Lukas, we've only seen this be an issue on server side errors, but I can't say for sure. It seems like a significant issue. At first it made us think we had a major security issue, we worried that somehow users were getting assigned other user's security credentials. In this specific case that I linked to, Sentry is claiming the user is my colleague here in Canada, but in reality it's a user of ours from France

…

On Fri, Mar 3, 2023, 12:18 AM Lukas Stracke ***@***.***> wrote: Hi @alexblack <https://github.com/alexblack>, thanks for writing in! Can you confirm that this issue only concerns server-side errors? (we're handling IP addresses differently for browser and server SDKs, which is the reason I'm asking) The reason why this is happening is that we're not looking at headers for determining the ip address. In fact, we take the IP we receive from the node request: https://github.com/getsentry/sentry-javascript/blob/a88a8bf0f26d12adde87738a3c9f56658397af9a/packages/node/src/requestdata.ts#L285-L296 I'm not sure if we're going to start analyzing headers as well, given that there's probably quite a few headers out there we'd potentially need to check. I'm gonna raise this internally to see if and how we'll make changes here. — Reply to this email directly, view it on GitHub <#7323 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABELQS4A2OLKFZH5KZGQ2LW2GSPHANCNFSM6AAAAAAVN5EVDU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Lms24]

Oh, I just realized we actually extract the IP from Http headers in the Remix SDK already (#6263). Sorry for the confusion. We'll need to look into this.
Would you be able to provide a minmal reproduction for this example?

I might have an idea what's going wrong but it's hard to verify without a reproduction.

[Lms24]

I opened #7329 with a potential fix. Would you mind taking a look at this PR if the fix makes sense to you? As I said it's hard to actually verify that this actually works.

[alexblack]

Thanks, that fix looks promising. I'm not sure how to reproduce it. Is there no way for you to "replay" the event on your end?

…

On Fri, Mar 03, 2023 at 6:58 AM, Lukas Stracke ***@***.***> wrote: I opened #7329 <#7329> with a potential fix. Would you mind taking a look at this PR if the fix makes sense to you? As I said it's hard to actually verify that this actually works. — Reply to this email directly, view it on GitHub <#7323 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABELQSNGIND5TGY423X4YDW2IBJBANCNFSM6AAAAAAVN5EVDU> . You are receiving this because you were mentioned.Message ID: ***@***.***>