Skip to content

Commit d039640

Browse files
Lms24Lms24
authored
fix(core): Ensure builtin stack frames don't affect thirdPartyErrorFilterIntegration (#17693)
As reported in #17674 our previous filter to exclude builtin or native stack frames from `thirdPartyErrorFilterIntegration` filtering logic would incorrectly still let some native frames slip through. This PR expands the check and lets the integration only consider frames with a filename (as previously) AND with at least a `lineno` or `colno`. If neither of these properties are present, we exclude them from being a factor in determining 1st vs. 3rd party. Also added integration tests for `thirdPartyErrorsFilterIntegration`. closes #17674
1 parent 5a1faed commit d039640

File tree

7 files changed

+211
-16
lines changed

7 files changed

+211
-16
lines changed

‎dev-packages/browser-integration-tests/suites/integrations/thirdPartyErrorsFilter/init.js‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import * as Sentry from '@sentry/browser';
2+
// eslint-disable-next-line import/no-duplicates
3+
import { thirdPartyErrorFilterIntegration } from '@sentry/browser';
4+
// eslint-disable-next-line import/no-duplicates
5+
import { captureConsoleIntegration } from '@sentry/browser';
6+
7+
// This is the code the bundler plugin would inject to mark the init bundle as a first party module:
8+
var _sentryModuleMetadataGlobal =
9+
typeof window !== 'undefined'
10+
? window
11+
: typeof global !== 'undefined'
12+
? global
13+
: typeof self !== 'undefined'
14+
? self
15+
: {};
16+
17+
_sentryModuleMetadataGlobal._sentryModuleMetadata = _sentryModuleMetadataGlobal._sentryModuleMetadata || {};
18+
19+
_sentryModuleMetadataGlobal._sentryModuleMetadata[new Error().stack] = Object.assign(
20+
{},
21+
_sentryModuleMetadataGlobal._sentryModuleMetadata[new Error().stack],
22+
{
23+
'_sentryBundlerPluginAppKey:my-app': true,
24+
},
25+
);
26+
27+
Sentry.init({
28+
dsn: 'https://[email protected]/1337',
29+
integrations: [
30+
thirdPartyErrorFilterIntegration({ behaviour: 'apply-tag-if-contains-third-party-frames', filterKeys: ['my-app'] }),
31+
captureConsoleIntegration({ levels: ['error'], handled: false }),
32+
],
33+
attachStacktrace: true,
34+
});

‎dev-packages/browser-integration-tests/suites/integrations/thirdPartyErrorsFilter/subject.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// This is the code the bundler plugin would inject to mark the subject bundle as a first party module:
2+
var _sentryModuleMetadataGlobal =
3+
typeof window !== 'undefined'
4+
? window
5+
: typeof global !== 'undefined'
6+
? global
7+
: typeof self !== 'undefined'
8+
? self
9+
: {};
10+
11+
_sentryModuleMetadataGlobal._sentryModuleMetadata = _sentryModuleMetadataGlobal._sentryModuleMetadata || {};
12+
13+
_sentryModuleMetadataGlobal._sentryModuleMetadata[new Error().stack] = Object.assign(
14+
{},
15+
_sentryModuleMetadataGlobal._sentryModuleMetadata[new Error().stack],
16+
{
17+
'_sentryBundlerPluginAppKey:my-app': true,
18+
},
19+
);
20+
21+
const errorBtn = document.getElementById('errBtn');
22+
errorBtn.addEventListener('click', async () => {
23+
Promise.allSettled([Promise.reject('I am a first party Error')]).then(values =>
24+
values.forEach(value => {
25+
if (value.status === 'rejected') console.error(value.reason);
26+
}),
27+
);
28+
});

‎dev-packages/browser-integration-tests/suites/integrations/thirdPartyErrorsFilter/template.html‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<!doctype html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<script src="thirdPartyScript.js"></script>
8+
<button id="errBtn">Throw 1st part yerror</button>
9+
</body>
10+
</html>

‎dev-packages/browser-integration-tests/suites/integrations/thirdPartyErrorsFilter/test.ts‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { readFileSync } from 'node:fs';
2+
import { join } from 'node:path';
3+
import { expect } from '@playwright/test';
4+
import { sentryTest } from '../../../utils/fixtures';
5+
import { envelopeRequestParser, waitForErrorRequest } from '../../../utils/helpers';
6+
7+
const bundle = process.env.PW_BUNDLE || '';
8+
// We only want to run this in non-CDN bundle mode because
9+
// thirdPartyErrorFilterIntegration is only available in the NPM package
10+
if (bundle.startsWith('bundle')) {
11+
sentryTest.skip();
12+
}
13+
14+
sentryTest('tags event if contains at least one third-party frame', async ({ getLocalTestUrl, page }) => {
15+
const url = await getLocalTestUrl({ testDir: __dirname });
16+
17+
const errorEventPromise = waitForErrorRequest(page, e => {
18+
return e.exception?.values?.[0]?.value === 'I am a third party Error';
19+
});
20+
21+
await page.route('**/thirdPartyScript.js', route =>
22+
route.fulfill({
23+
status: 200,
24+
body: readFileSync(join(__dirname, 'thirdPartyScript.js')),
25+
}),
26+
);
27+
28+
await page.goto(url);
29+
30+
const errorEvent = envelopeRequestParser(await errorEventPromise);
31+
expect(errorEvent.tags?.third_party_code).toBe(true);
32+
});
33+
34+
/**
35+
* This test seems a bit more complicated than necessary but this is intentional:
36+
* When using `captureConsoleIntegration` in combination with `thirdPartyErrorFilterIntegration`
37+
* and `attachStacktrace: true`, the stack trace includes native code stack frames which previously broke
38+
* the third party error filtering logic.
39+
*
40+
* see https://github.com/getsentry/sentry-javascript/issues/17674
41+
*/
42+
sentryTest(
43+
"doesn't tag event if doesn't contain third-party frames",
44+
async ({ getLocalTestUrl, page, browserName }) => {
45+
const url = await getLocalTestUrl({ testDir: __dirname });
46+
47+
const errorEventPromise = waitForErrorRequest(page, e => {
48+
return e.exception?.values?.[0]?.value === 'I am a first party Error';
49+
});
50+
51+
await page.route('**/thirdPartyScript.js', route =>
52+
route.fulfill({
53+
status: 200,
54+
body: readFileSync(join(__dirname, 'thirdPartyScript.js')),
55+
}),
56+
);
57+
58+
await page.goto(url);
59+
60+
await page.click('#errBtn');
61+
62+
const errorEvent = envelopeRequestParser(await errorEventPromise);
63+
64+
expect(errorEvent.tags?.third_party_code).toBeUndefined();
65+
66+
// ensure the stack trace includes native code stack frames which previously broke
67+
// the third party error filtering logic
68+
if (browserName === 'chromium') {
69+
expect(errorEvent.exception?.values?.[0]?.stacktrace?.frames).toContainEqual({
70+
filename: '<anonymous>',
71+
function: 'Array.forEach',
72+
in_app: true,
73+
});
74+
} else if (browserName === 'webkit') {
75+
expect(errorEvent.exception?.values?.[0]?.stacktrace?.frames).toContainEqual({
76+
filename: '[native code]',
77+
function: 'forEach',
78+
in_app: true,
79+
});
80+
}
81+
},
82+
);

‎dev-packages/browser-integration-tests/suites/integrations/thirdPartyErrorsFilter/thirdPartyScript.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
setTimeout(() => {
2+
throw new Error('I am a third party Error');
3+
}, 100);

‎packages/core/src/integrations/third-party-errors-filter.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,9 @@ function getBundleKeysForAllFramesWithFilenames(event: Event): string[][] | unde
108108

109109
return (
110110
frames
111-
// Exclude frames without a filename since these are likely native code or built-ins
112-
.filter(frame => !!frame.filename)
111+
// Exclude frames without a filename or without lineno and colno,
112+
// since these are likely native code or built-ins
113+
.filter(frame => !!frame.filename && (frame.lineno ?? frame.colno) != null)
113114
.map(frame => {
114115
if (frame.module_metadata) {
115116
return Object.keys(frame.module_metadata)

‎packages/core/test/lib/integrations/third-party-errors-filter.test.ts‎

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,19 @@ const eventWithThirdAndFirstPartyFrames: Event = {
3232
function: 'function',
3333
lineno: 2,
3434
},
35+
// The following frames are native/built-in frames which should be ignored by the integration
36+
{
37+
function: 'Array.forEach',
38+
filename: '<anonymous>',
39+
abs_path: '<anonymous>',
40+
in_app: true,
41+
},
42+
{
43+
function: 'async Promise.all',
44+
filename: 'index 1',
45+
abs_path: 'index 1',
46+
in_app: true,
47+
},
3548
],
3649
},
3750
type: 'SyntaxError',
@@ -51,14 +64,25 @@ const eventWithOnlyFirstPartyFrames: Event = {
5164
colno: 1,
5265
filename: __filename,
5366
function: 'function',
54-
lineno: 1,
5567
},
5668
{
57-
colno: 2,
5869
filename: __filename,
5970
function: 'function',
6071
lineno: 2,
6172
},
73+
// The following frames are native/built-in frames which should be ignored by the integration
74+
{
75+
function: 'Array.forEach',
76+
filename: '<anonymous>',
77+
abs_path: '<anonymous>',
78+
in_app: true,
79+
},
80+
{
81+
function: 'async Promise.all',
82+
filename: 'index 1',
83+
abs_path: 'index 1',
84+
in_app: true,
85+
},
6286
],
6387
},
6488
type: 'SyntaxError',
@@ -86,6 +110,19 @@ const eventWithOnlyThirdPartyFrames: Event = {
86110
function: 'function',
87111
lineno: 2,
88112
},
113+
// The following frames are native/built-in frames which should be ignored by the integration
114+
{
115+
function: 'Array.forEach',
116+
filename: '<anonymous>',
117+
abs_path: '<anonymous>',
118+
in_app: true,
119+
},
120+
{
121+
function: 'async Promise.all',
122+
filename: 'index 1',
123+
abs_path: 'index 1',
124+
in_app: true,
125+
},
89126
],
90127
},
91128
type: 'SyntaxError',
@@ -112,7 +149,7 @@ describe('ThirdPartyErrorFilter', () => {
112149
});
113150

114151
describe('drop-error-if-contains-third-party-frames', () => {
115-
it('should keep event if there are exclusively first-party frames', async () => {
152+
it('keeps event if there are exclusively first-party frames', async () => {
116153
const integration = thirdPartyErrorFilterIntegration({
117154
behaviour: 'drop-error-if-contains-third-party-frames',
118155
filterKeys: ['some-key'],
@@ -123,7 +160,7 @@ describe('ThirdPartyErrorFilter', () => {
123160
expect(result).toBeDefined();
124161
});
125162

126-
it('should drop event if there is at least one third-party frame', async () => {
163+
it('drops event if there is at least one third-party frame', async () => {
127164
const integration = thirdPartyErrorFilterIntegration({
128165
behaviour: 'drop-error-if-contains-third-party-frames',
129166
filterKeys: ['some-key'],
@@ -134,7 +171,7 @@ describe('ThirdPartyErrorFilter', () => {
134171
expect(result).toBe(null);
135172
});
136173

137-
it('should drop event if all frames are third-party frames', async () => {
174+
it('drops event if all frames are third-party frames', async () => {
138175
const integration = thirdPartyErrorFilterIntegration({
139176
behaviour: 'drop-error-if-contains-third-party-frames',
140177
filterKeys: ['some-key'],
@@ -147,7 +184,7 @@ describe('ThirdPartyErrorFilter', () => {
147184
});
148185

149186
describe('drop-error-if-exclusively-contains-third-party-frames', () => {
150-
it('should keep event if there are exclusively first-party frames', async () => {
187+
it('keeps event if there are exclusively first-party frames', async () => {
151188
const integration = thirdPartyErrorFilterIntegration({
152189
behaviour: 'drop-error-if-exclusively-contains-third-party-frames',
153190
filterKeys: ['some-key'],
@@ -158,7 +195,7 @@ describe('ThirdPartyErrorFilter', () => {
158195
expect(result).toBeDefined();
159196
});
160197

161-
it('should keep event if there is at least one first-party frame', async () => {
198+
it('keeps event if there is at least one first-party frame', async () => {
162199
const integration = thirdPartyErrorFilterIntegration({
163200
behaviour: 'drop-error-if-exclusively-contains-third-party-frames',
164201
filterKeys: ['some-key'],
@@ -169,7 +206,7 @@ describe('ThirdPartyErrorFilter', () => {
169206
expect(result).toBeDefined();
170207
});
171208

172-
it('should drop event if all frames are third-party frames', async () => {
209+
it('drops event if all frames are third-party frames', async () => {
173210
const integration = thirdPartyErrorFilterIntegration({
174211
behaviour: 'drop-error-if-exclusively-contains-third-party-frames',
175212
filterKeys: ['some-key'],
@@ -182,7 +219,7 @@ describe('ThirdPartyErrorFilter', () => {
182219
});
183220

184221
describe('apply-tag-if-contains-third-party-frames', () => {
185-
it('should not tag event if exclusively contains first-party frames', async () => {
222+
it("doesn't tag event if exclusively contains first-party frames", async () => {
186223
const integration = thirdPartyErrorFilterIntegration({
187224
behaviour: 'apply-tag-if-contains-third-party-frames',
188225
filterKeys: ['some-key'],
@@ -193,7 +230,7 @@ describe('ThirdPartyErrorFilter', () => {
193230
expect(result?.tags?.third_party_code).toBeUndefined();
194231
});
195232

196-
it('should tag event if contains at least one third-party frame', async () => {
233+
it('tags event if contains at least one third-party frame', async () => {
197234
const integration = thirdPartyErrorFilterIntegration({
198235
behaviour: 'apply-tag-if-contains-third-party-frames',
199236
filterKeys: ['some-key'],
@@ -204,7 +241,7 @@ describe('ThirdPartyErrorFilter', () => {
204241
expect(result?.tags).toMatchObject({ third_party_code: true });
205242
});
206243

207-
it('should tag event if contains exclusively third-party frames', async () => {
244+
it('tags event if contains exclusively third-party frames', async () => {
208245
const integration = thirdPartyErrorFilterIntegration({
209246
behaviour: 'apply-tag-if-contains-third-party-frames',
210247
filterKeys: ['some-key'],
@@ -217,7 +254,7 @@ describe('ThirdPartyErrorFilter', () => {
217254
});
218255

219256
describe('apply-tag-if-exclusively-contains-third-party-frames', () => {
220-
it('should not tag event if exclusively contains first-party frames', async () => {
257+
it("doesn't tag event if exclusively contains first-party frames", async () => {
221258
const integration = thirdPartyErrorFilterIntegration({
222259
behaviour: 'apply-tag-if-exclusively-contains-third-party-frames',
223260
filterKeys: ['some-key'],
@@ -228,7 +265,7 @@ describe('ThirdPartyErrorFilter', () => {
228265
expect(result?.tags?.third_party_code).toBeUndefined();
229266
});
230267

231-
it('should not tag event if contains at least one first-party frame', async () => {
268+
it("doesn't tag event if contains at least one first-party frame", async () => {
232269
const integration = thirdPartyErrorFilterIntegration({
233270
behaviour: 'apply-tag-if-exclusively-contains-third-party-frames',
234271
filterKeys: ['some-key'],
@@ -239,7 +276,7 @@ describe('ThirdPartyErrorFilter', () => {
239276
expect(result?.tags?.third_party_code).toBeUndefined();
240277
});
241278

242-
it('should tag event if contains exclusively third-party frames', async () => {
279+
it('tags event if contains exclusively third-party frames', async () => {
243280
const integration = thirdPartyErrorFilterIntegration({
244281
behaviour: 'apply-tag-if-exclusively-contains-third-party-frames',
245282
filterKeys: ['some-key'],

0 commit comments

Comments
 (0)