From 197dac06863fa9a8f0cebee87962c26d032cba10 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 2 Dec 2024 18:03:13 +0100 Subject: [PATCH 1/8] fix(core): Decode file name in Node stack parser --- packages/core/src/utils-hoist/node-stack-trace.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/utils-hoist/node-stack-trace.ts b/packages/core/src/utils-hoist/node-stack-trace.ts index 946492ede855..46f036cb0270 100644 --- a/packages/core/src/utils-hoist/node-stack-trace.ts +++ b/packages/core/src/utils-hoist/node-stack-trace.ts @@ -113,7 +113,7 @@ export function node(getModule?: GetModuleFn): StackLineParserFn { } return { - filename, + filename: filename ? decodeURI(filename) : undefined, module: getModule ? getModule(filename) : undefined, function: functionName, lineno: _parseIntOrUndefined(lineMatch[3]), From d1c2f2e792bb5c2d41f4a4ca6c1e69e978ae43ef Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 2 Dec 2024 18:06:42 +0100 Subject: [PATCH 2/8] add esm integration test --- .../suites/contextLines/instrument.mjs | 10 +++++ .../contextLines/scenario with space.mjs | 5 +++ .../suites/contextLines/test.ts | 39 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 dev-packages/node-integration-tests/suites/contextLines/instrument.mjs create mode 100644 dev-packages/node-integration-tests/suites/contextLines/scenario with space.mjs create mode 100644 dev-packages/node-integration-tests/suites/contextLines/test.ts diff --git a/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs b/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs new file mode 100644 index 000000000000..599f86c7a925 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs @@ -0,0 +1,10 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + autoSessionTracking: false, + transport: loggingTransport, + debug: true, +}); diff --git a/dev-packages/node-integration-tests/suites/contextLines/scenario with space.mjs b/dev-packages/node-integration-tests/suites/contextLines/scenario with space.mjs new file mode 100644 index 000000000000..ce9b5ee327df --- /dev/null +++ b/dev-packages/node-integration-tests/suites/contextLines/scenario with space.mjs @@ -0,0 +1,5 @@ +import * as Sentry from '@sentry/node'; + +Sentry.captureException(new Error('Test Error')); + +// some more post context diff --git a/dev-packages/node-integration-tests/suites/contextLines/test.ts b/dev-packages/node-integration-tests/suites/contextLines/test.ts new file mode 100644 index 000000000000..b785430fd7ea --- /dev/null +++ b/dev-packages/node-integration-tests/suites/contextLines/test.ts @@ -0,0 +1,39 @@ +import { join } from 'path'; +import { createRunner } from '../../utils/runner'; + +describe('ContextLines integration', () => { + test('reads context lines from filenames with spaces', done => { + expect.assertions(1); + const instrumentPath = join(__dirname, 'instrument.mjs'); + + createRunner(__dirname, 'scenario with space.mjs') + .withFlags('--import', instrumentPath) + .expect({ + event: { + exception: { + values: [ + { + value: 'Test Error', + stacktrace: { + frames: expect.arrayContaining([ + { + filename: expect.stringMatching(/\/scenario with space.mjs$/), + context_line: "Sentry.captureException(new Error('Test Error'));", + pre_context: ["import * as Sentry from '@sentry/node';", ''], + post_context: ['', '// some more post context'], + colno: 25, + lineno: 3, + function: '?', + in_app: true, + module: 'scenario%20with%20space', + }, + ]), + }, + }, + ], + }, + }, + }) + .start(done); + }); +}); From 42dd055a6bf818628fbeaef67f9a848369dab638 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 3 Dec 2024 16:17:11 +0100 Subject: [PATCH 3/8] add more tests --- .../suites/contextLines/instrument.mjs | 1 - .../contextLines/scenario with space.cjs | 13 ++++ .../suites/contextLines/test.ts | 43 ++++++++++++- .../core/test/utils-hoist/stacktrace.test.ts | 60 +++++++++++++++++++ packages/node/src/utils/module.ts | 10 +--- packages/node/test/utils/module.test.ts | 41 +++++++++++++ 6 files changed, 158 insertions(+), 10 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/contextLines/scenario with space.cjs create mode 100644 packages/node/test/utils/module.test.ts diff --git a/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs b/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs index 599f86c7a925..430132040b41 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs @@ -6,5 +6,4 @@ Sentry.init({ release: '1.0', autoSessionTracking: false, transport: loggingTransport, - debug: true, }); diff --git a/dev-packages/node-integration-tests/suites/contextLines/scenario with space.cjs b/dev-packages/node-integration-tests/suites/contextLines/scenario with space.cjs new file mode 100644 index 000000000000..9e9c52cd0928 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/contextLines/scenario with space.cjs @@ -0,0 +1,13 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + autoSessionTracking: false, + transport: loggingTransport, +}); + +Sentry.captureException(new Error('Test Error')); + +// some more post context diff --git a/dev-packages/node-integration-tests/suites/contextLines/test.ts b/dev-packages/node-integration-tests/suites/contextLines/test.ts index b785430fd7ea..f4e3c10eb972 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/test.ts +++ b/dev-packages/node-integration-tests/suites/contextLines/test.ts @@ -2,7 +2,7 @@ import { join } from 'path'; import { createRunner } from '../../utils/runner'; describe('ContextLines integration', () => { - test('reads context lines from filenames with spaces', done => { + test('reads encoded context lines from filenames with spaces (ESM)', done => { expect.assertions(1); const instrumentPath = join(__dirname, 'instrument.mjs'); @@ -36,4 +36,45 @@ describe('ContextLines integration', () => { }) .start(done); }); + + test('reads context lines from filenames with spaces (CJS)', done => { + expect.assertions(1); + + createRunner(__dirname, 'scenario with space.cjs') + .expect({ + event: { + exception: { + values: [ + { + value: 'Test Error', + stacktrace: { + frames: expect.arrayContaining([ + { + filename: expect.stringMatching(/\/scenario with space.cjs$/), + context_line: "Sentry.captureException(new Error('Test Error'));", + pre_context: [ + 'Sentry.init({', + " dsn: 'https://public@dsn.ingest.sentry.io/1337',", + " release: '1.0',", + ' autoSessionTracking: false,', + ' transport: loggingTransport,', + '});', + '', + ], + post_context: ['', '// some more post context'], + colno: 25, + lineno: 11, + function: 'Object.?', + in_app: true, + module: 'scenario with space', + }, + ]), + }, + }, + ], + }, + }, + }) + .start(done); + }); }); diff --git a/packages/core/test/utils-hoist/stacktrace.test.ts b/packages/core/test/utils-hoist/stacktrace.test.ts index 22a50f3f71de..1732c35d02bc 100644 --- a/packages/core/test/utils-hoist/stacktrace.test.ts +++ b/packages/core/test/utils-hoist/stacktrace.test.ts @@ -319,4 +319,64 @@ describe('node', () => { const result = node(line); expect(result?.in_app).toBe(true); }); + + it('parses frame filename paths with spaces and characters in file name', () => { + const input = 'at myObject.myMethod (/path/to/file with space(1).js:10:5)'; + + const expectedOutput = { + filename: '/path/to/file with space(1).js', + module: undefined, + function: 'myObject.myMethod', + lineno: 10, + colno: 5, + in_app: true, + }; + + expect(node(input)).toEqual(expectedOutput); + }); + + it('parses frame filename paths with spaces and characters in file path', () => { + const input = 'at myObject.myMethod (/path with space(1)/to/file.js:10:5)'; + + const expectedOutput = { + filename: '/path with space(1)/to/file.js', + module: undefined, + function: 'myObject.myMethod', + lineno: 10, + colno: 5, + in_app: true, + }; + + expect(node(input)).toEqual(expectedOutput); + }); + + it('parses encoded frame filename paths with spaces and characters in file name', () => { + const input = 'at myObject.myMethod (/path/to/file%20with%20space(1).js:10:5)'; + + const expectedOutput = { + filename: '/path/to/file with space(1).js', + module: undefined, + function: 'myObject.myMethod', + lineno: 10, + colno: 5, + in_app: true, + }; + + expect(node(input)).toEqual(expectedOutput); + }); + + it('parses encoded frame filename paths with spaces and characters in file path', () => { + const input = 'at myObject.myMethod (/path%20with%20space(1)/to/file.js:10:5)'; + + const expectedOutput = { + filename: '/path with space(1)/to/file.js', + module: undefined, + function: 'myObject.myMethod', + lineno: 10, + colno: 5, + in_app: true, + }; + + expect(node(input)).toEqual(expectedOutput); + }); }); diff --git a/packages/node/src/utils/module.ts b/packages/node/src/utils/module.ts index b5470269b921..3c423aa273b9 100644 --- a/packages/node/src/utils/module.ts +++ b/packages/node/src/utils/module.ts @@ -42,14 +42,8 @@ export function createGetModuleFromFilename( // Let's see if it's a part of the main module // To be a part of main module, it has to share the same base if (dir.startsWith(normalizedBase)) { - let moduleName = dir.slice(normalizedBase.length + 1).replace(/\//g, '.'); - - if (moduleName) { - moduleName += ':'; - } - moduleName += file; - - return moduleName; + const moduleName = dir.slice(normalizedBase.length + 1).replace(/\//g, '.'); + return moduleName ? `${moduleName}:${file}` : file; } return file; diff --git a/packages/node/test/utils/module.test.ts b/packages/node/test/utils/module.test.ts new file mode 100644 index 000000000000..8335604962f1 --- /dev/null +++ b/packages/node/test/utils/module.test.ts @@ -0,0 +1,41 @@ +import { createGetModuleFromFilename } from '../../src'; + +describe('createGetModuleFromFilename', () => { + it.each([ + ['/path/to/file.js', 'file'], + ['/path/to/file.mjs', 'file'], + ['/path/to/file.cjs', 'file'], + ['file.js', 'file'], + ])('returns the module name from a filename %s', (filename, expected) => { + const getModule = createGetModuleFromFilename(); + expect(getModule(filename)).toBe(expected); + }); + + it('applies the given base path', () => { + const getModule = createGetModuleFromFilename('/path/to/base'); + expect(getModule('/path/to/base/file.js')).toBe('file'); + }); + + it('returns undefined if no filename is provided', () => { + const getModule = createGetModuleFromFilename(); + expect(getModule(undefined)).toBeUndefined(); + }); + + it.each([ + ['/path/to/base/node_modules/@sentry/test/file.js', '@sentry.test:file'], + ['/path/to/base/node_modules/somePkg/file.js', 'somePkg:file'], + ])('handles node_modules file paths %s', (filename, expected) => { + const getModule = createGetModuleFromFilename(); + expect(getModule(filename)).toBe(expected); + }); + + it('handles windows paths with passed basePath', () => { + const getModule = createGetModuleFromFilename('C:\\path\\to\\base', true); + expect(getModule('C:\\path\\to\\base\\node_modules\\somePkg\\file.js')).toBe('somePkg:file'); + }); + + it('handles windows paths with default basePath', () => { + const getModule = createGetModuleFromFilename(undefined, true); + expect(getModule('C:\\path\\to\\base\\somePkg\\file.js')).toBe('file'); + }); +}); From 3174ae2ae6f3306aa13a45ccbeb9ce121ee92773 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 3 Dec 2024 16:58:19 +0100 Subject: [PATCH 4/8] also decode module field --- .../node-integration-tests/suites/contextLines/test.ts | 2 +- packages/node/src/utils/module.ts | 10 +++++++--- packages/node/test/utils/module.test.ts | 7 ++++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/contextLines/test.ts b/dev-packages/node-integration-tests/suites/contextLines/test.ts index f4e3c10eb972..4248a6a449f7 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/test.ts +++ b/dev-packages/node-integration-tests/suites/contextLines/test.ts @@ -25,7 +25,7 @@ describe('ContextLines integration', () => { lineno: 3, function: '?', in_app: true, - module: 'scenario%20with%20space', + module: 'scenario with space', }, ]), }, diff --git a/packages/node/src/utils/module.ts b/packages/node/src/utils/module.ts index 3c423aa273b9..c7ea0ffee30a 100644 --- a/packages/node/src/utils/module.ts +++ b/packages/node/src/utils/module.ts @@ -29,6 +29,10 @@ export function createGetModuleFromFilename( file = file.slice(0, ext.length * -1); } + // The file name might be URI-encoded which we want to decode to + // the original file name. + const decodedFile = decodeURIComponent(file); + if (!dir) { // No dirname whatsoever dir = '.'; @@ -36,16 +40,16 @@ export function createGetModuleFromFilename( const n = dir.lastIndexOf('/node_modules'); if (n > -1) { - return `${dir.slice(n + 14).replace(/\//g, '.')}:${file}`; + return `${dir.slice(n + 14).replace(/\//g, '.')}:${decodedFile}`; } // Let's see if it's a part of the main module // To be a part of main module, it has to share the same base if (dir.startsWith(normalizedBase)) { const moduleName = dir.slice(normalizedBase.length + 1).replace(/\//g, '.'); - return moduleName ? `${moduleName}:${file}` : file; + return moduleName ? `${moduleName}:${decodedFile}` : decodedFile; } - return file; + return decodedFile; }; } diff --git a/packages/node/test/utils/module.test.ts b/packages/node/test/utils/module.test.ts index 8335604962f1..1616bc7482d9 100644 --- a/packages/node/test/utils/module.test.ts +++ b/packages/node/test/utils/module.test.ts @@ -16,6 +16,11 @@ describe('createGetModuleFromFilename', () => { expect(getModule('/path/to/base/file.js')).toBe('file'); }); + it('decodes URI-encoded file names', () => { + const getModule = createGetModuleFromFilename(); + expect(getModule('/path%20with%space/file%20with%20spaces(1).js')).toBe('file with spaces(1)'); + }); + it('returns undefined if no filename is provided', () => { const getModule = createGetModuleFromFilename(); expect(getModule(undefined)).toBeUndefined(); @@ -29,7 +34,7 @@ describe('createGetModuleFromFilename', () => { expect(getModule(filename)).toBe(expected); }); - it('handles windows paths with passed basePath', () => { + it('handles windows paths with passed basePath and node_modules', () => { const getModule = createGetModuleFromFilename('C:\\path\\to\\base', true); expect(getModule('C:\\path\\to\\base\\node_modules\\somePkg\\file.js')).toBe('somePkg:file'); }); From 5000272a1851224745ad2d99e2de0b4289693068 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 3 Dec 2024 16:58:49 +0100 Subject: [PATCH 5/8] formatting --- .../node-integration-tests/suites/contextLines/instrument.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs b/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs index 430132040b41..b3b8dda3720c 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/contextLines/instrument.mjs @@ -1,5 +1,5 @@ -import * as Sentry from '@sentry/node'; import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', From 6d7f1968bf7f2210133fef944762ef65ad3c3fc2 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 3 Dec 2024 17:13:11 +0100 Subject: [PATCH 6/8] skip esm for Node 14, 16 --- .../node-integration-tests/suites/contextLines/test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/contextLines/test.ts b/dev-packages/node-integration-tests/suites/contextLines/test.ts index 4248a6a449f7..c2a57393d9cf 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/test.ts +++ b/dev-packages/node-integration-tests/suites/contextLines/test.ts @@ -1,8 +1,9 @@ import { join } from 'path'; import { createRunner } from '../../utils/runner'; +import { conditionalTest } from '../../utils'; -describe('ContextLines integration', () => { - test('reads encoded context lines from filenames with spaces (ESM)', done => { +describe('ContextLines integration in CJS', () => { + test('reads encoded context lines from filenames with spaces', done => { expect.assertions(1); const instrumentPath = join(__dirname, 'instrument.mjs'); @@ -36,8 +37,10 @@ describe('ContextLines integration', () => { }) .start(done); }); +}); - test('reads context lines from filenames with spaces (CJS)', done => { +conditionalTest({ min: 18 })('ContextLines integration in ESM', () => { + test('reads context lines from filenames with spaces', done => { expect.assertions(1); createRunner(__dirname, 'scenario with space.cjs') From 09fb5c5f7fbbb30f8ed54582af1d675cae1c47fd Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 3 Dec 2024 17:18:16 +0100 Subject: [PATCH 7/8] formatting once more --- dev-packages/node-integration-tests/suites/contextLines/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/node-integration-tests/suites/contextLines/test.ts b/dev-packages/node-integration-tests/suites/contextLines/test.ts index c2a57393d9cf..12d957b5ac04 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/test.ts +++ b/dev-packages/node-integration-tests/suites/contextLines/test.ts @@ -1,6 +1,6 @@ import { join } from 'path'; -import { createRunner } from '../../utils/runner'; import { conditionalTest } from '../../utils'; +import { createRunner } from '../../utils/runner'; describe('ContextLines integration in CJS', () => { test('reads encoded context lines from filenames with spaces', done => { From 681b591cca13c6ac7c82f3c2678eee917152b69a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 4 Dec 2024 17:06:26 +0100 Subject: [PATCH 8/8] fix integration tests --- .../node-integration-tests/suites/contextLines/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/contextLines/test.ts b/dev-packages/node-integration-tests/suites/contextLines/test.ts index 12d957b5ac04..1912f0b57f04 100644 --- a/dev-packages/node-integration-tests/suites/contextLines/test.ts +++ b/dev-packages/node-integration-tests/suites/contextLines/test.ts @@ -2,7 +2,7 @@ import { join } from 'path'; import { conditionalTest } from '../../utils'; import { createRunner } from '../../utils/runner'; -describe('ContextLines integration in CJS', () => { +conditionalTest({ min: 18 })('ContextLines integration in ESM', () => { test('reads encoded context lines from filenames with spaces', done => { expect.assertions(1); const instrumentPath = join(__dirname, 'instrument.mjs'); @@ -39,7 +39,7 @@ describe('ContextLines integration in CJS', () => { }); }); -conditionalTest({ min: 18 })('ContextLines integration in ESM', () => { +describe('ContextLines integration in CJS', () => { test('reads context lines from filenames with spaces', done => { expect.assertions(1);