Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
# Changelog

## Unreleased

### Important Changes

- **fix(browser): Ensure IP address is only inferred by Relay if `sendDefaultPii` is `true`** ([#390](https://github.com/getsentry/sentry-cordova/pull/390))

This release includes a fix for a behaviour change
that was originally introduced with the newer JavaScript SDK: User IP Addresses should only be added to Sentry events automatically,
if `sendDefaultPii` was set to `true`.

To avoid making a major bump, the fix was patched on the current version and not by bumping to V10.
There is _no API_ breakage involved and hence it is safe to update.
However, after updating the SDK, events (errors, traces, replays, etc.) sent from the browser, will only include
user IP addresses, if you set `sendDefaultPii: true` in your `Sentry.init` options.

We apologize for any inconvenience caused!

## 1.6.0

### Fixes
Expand Down
95 changes: 95 additions & 0 deletions src/js/__tests__/integrations/sdkinfo.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { addEventProcessor, getClient } from '@sentry/core';

import { SdkInfo } from '../../integrations/sdkinfo';
import { SDK_NAME, SDK_VERSION } from '../../version';

jest.mock('@sentry/core', () => ({
addEventProcessor: jest.fn(),
getClient: jest.fn(),
}));

describe('SdkInfo Integration', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('should have correct id and name', () => {
const sdkInfo = new SdkInfo();
expect(SdkInfo.id).toBe('SdkInfo');
expect(sdkInfo.name).toBe('SdkInfo');
});

it('should add an event processor', () => {
(getClient as jest.Mock).mockReturnValue({
getOptions: () => ({ sendDefaultPii: true }),
});

const sdkInfo = new SdkInfo();
sdkInfo.setupOnce();

expect(addEventProcessor).toHaveBeenCalled();
});

it('should patch event sdk info correctly with sendDefaultPii true', async () => {
(getClient as jest.Mock).mockReturnValue({
getOptions: () => ({ sendDefaultPii: true }),
});

const sdkInfo = new SdkInfo();
sdkInfo.setupOnce();

const eventProcessor = (addEventProcessor as jest.Mock).mock.calls[0][0];

const event = { sdk: { packages: [] } };
const processedEvent = await eventProcessor(event);

expect(processedEvent.platform).toBe('javascript');
expect(processedEvent.sdk.name).toBe(SDK_NAME);
expect(processedEvent.sdk.version).toBe(SDK_VERSION);
expect(processedEvent.sdk.packages).toContainEqual({
name: 'npm:sentry-cordova',
version: SDK_VERSION,
});
expect(processedEvent.sdk.settings?.infer_ip).toBe('auto');
});

it('should patch event sdk info correctly with sendDefaultPii false', async () => {
(getClient as jest.Mock).mockReturnValue({
getOptions: () => ({ sendDefaultPii: false }),
});

const sdkInfo = new SdkInfo();
sdkInfo.setupOnce();

const eventProcessor = (addEventProcessor as jest.Mock).mock.calls[0][0];

const event = { sdk: {} };
const processedEvent = await eventProcessor(event);

expect(processedEvent.sdk.settings?.infer_ip).toBe('never');
});

it('should preserve existing sdk settings', async () => {
(getClient as jest.Mock).mockReturnValue({
getOptions: () => ({ sendDefaultPii: true }),
});

const sdkInfo = new SdkInfo();
sdkInfo.setupOnce();

const eventProcessor = (addEventProcessor as jest.Mock).mock.calls[0][0];

const event = {
sdk: {
packages: [],
settings: { debug: true },
},
};
const processedEvent = await eventProcessor(event);

expect(processedEvent.sdk.settings).toEqual({
infer_ip: 'auto',
debug: true,
});
});
});
49 changes: 36 additions & 13 deletions src/js/integrations/sdkinfo.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { addEventProcessor } from '@sentry/core';
import type { Integration } from '@sentry/types';
import { addEventProcessor, getClient } from '@sentry/core';
import type { Integration, SdkInfo as SdkInfoType} from '@sentry/types';

import { SDK_NAME, SDK_VERSION } from '../version';

interface IpPatchedSdkInfo extends SdkInfoType {
settings?: {
infer_ip?: 'auto' | 'never';
};
}

/** Default SdkInfo instrumentation */
export class SdkInfo implements Integration {
/**
Expand All @@ -19,21 +25,38 @@ export class SdkInfo implements Integration {
* @inheritDoc
*/
public setupOnce(): void {

let defaultPii: boolean | undefined = undefined;

const client = getClient();
if (client) {
const options = client?.getOptions();
defaultPii = options.sendDefaultPii;
}

Choose a reason for hiding this comment

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

Potential bug: The SdkInfo integration caches the sendDefaultPii option during initial setup. If Sentry.init() is called again, the integration uses a stale value, leading to incorrect IP inference.
  • Description: The SdkInfo integration captures the sendDefaultPii option value within the setupOnce method. Because setupOnce is designed to run only once per integration lifecycle, this value becomes stale if the Sentry SDK is re-initialized with different options via a subsequent Sentry.init() call. For example, if an application initializes with sendDefaultPii: false and later re-initializes with sendDefaultPii: true after obtaining user consent, the event processor will continue to use the initial false value. This results in infer_ip being incorrectly set to 'never', causing a loss of IP address data.

  • Suggested fix: To ensure the most current options are always used, move the client and option retrieval logic from setupOnce into the addEventProcessor callback. This will fetch getClient().getOptions() for each event, reflecting any changes from SDK re-initialization.
    severity: 0.65, confidence: 0.9

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Member

Choose a reason for hiding this comment

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

this isn't wrong but we don't guarantee mutability of SDK options and JS has the same behaviour
(happy to leave the call up to you if you wanna change it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The SDK doesn't behave well from what I tested on multiple initializations, and since the SDK is on low maintenance I'd say it's good enough.

addEventProcessor(async (event) => {
event.platform = event.platform || 'javascript';
event.sdk = {
...event.sdk,
name: SDK_NAME,
packages: [
...((event.sdk && event.sdk.packages) || []),
{
name: 'npm:sentry-cordova',
version: SDK_VERSION,
},
],
version: SDK_VERSION,
const sdk = (event.sdk || {}) as IpPatchedSdkInfo;

sdk.name = sdk.name || SDK_NAME;
sdk.packages = [
...((event.sdk && event.sdk.packages) || []),
{
name: 'npm:sentry-cordova',
version: SDK_VERSION,
},
];
sdk.version = SDK_VERSION;

// Patch missing infer_ip.
sdk.settings = {
infer_ip: defaultPii ? 'auto' : 'never',
// purposefully allowing already passed settings to override the default
...sdk.settings
};

event.sdk = sdk;

return event;
});
}
Expand Down
Loading