From ee8c7f53f916c983c1f7cc139485a2f3939716a1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 18 Mar 2025 11:18:19 +0100 Subject: [PATCH 1/3] fix(node): Use `fatal` level for unhandled rejections in `strict` mode --- .../mode-none.js | 13 ++ .../mode-strict.js | 14 ++ .../mode-warn-error.js | 12 ++ .../mode-warn-string.js | 12 ++ .../scenario-strict.ts | 12 ++ .../scenario-warn.ts | 11 ++ .../onUnhandledRejectionIntegration/test.ts | 127 ++++++++++++++++++ .../src/integrations/onunhandledrejection.ts | 20 ++- 8 files changed, 214 insertions(+), 7 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-none.js create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-strict.js create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-error.js create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-string.js create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-strict.ts create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-warn.ts create mode 100644 dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-none.js b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-none.js new file mode 100644 index 000000000000..468b80a2df5e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-none.js @@ -0,0 +1,13 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [Sentry.onUnhandledRejectionIntegration({ mode: 'none' })], +}); + +setTimeout(() => { + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +Promise.reject('test rejection'); diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-strict.js b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-strict.js new file mode 100644 index 000000000000..76c18bbdc51e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-strict.js @@ -0,0 +1,14 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [Sentry.onUnhandledRejectionIntegration({ mode: 'strict' })], +}); + +setTimeout(() => { + // should not be called + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +Promise.reject('test rejection'); diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-error.js b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-error.js new file mode 100644 index 000000000000..57566677cd7a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-error.js @@ -0,0 +1,12 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', +}); + +setTimeout(() => { + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +Promise.reject(new Error('test rejection')); diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-string.js b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-string.js new file mode 100644 index 000000000000..5d45dbde5870 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/mode-warn-string.js @@ -0,0 +1,12 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', +}); + +setTimeout(() => { + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +Promise.reject('test rejection'); diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-strict.ts b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-strict.ts new file mode 100644 index 000000000000..9da994967d58 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-strict.ts @@ -0,0 +1,12 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, + integrations: [Sentry.onUnhandledRejectionIntegration({ mode: 'strict' })], +}); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Promise.reject('test rejection'); diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-warn.ts b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-warn.ts new file mode 100644 index 000000000000..55b283af81ae --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/scenario-warn.ts @@ -0,0 +1,11 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, +}); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Promise.reject('test rejection'); diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts new file mode 100644 index 000000000000..4c99f1b02052 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts @@ -0,0 +1,127 @@ +import * as childProcess from 'child_process'; +import * as path from 'path'; +import { afterAll, describe, expect, test } from 'vitest'; +import { cleanupChildProcesses } from '../../../utils/runner'; +import { createRunner } from '../../../utils/runner'; + +describe('onUnhandledRejectionIntegration', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('should show string-type promise rejection warnings by default', () => + new Promise(done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'mode-warn-string.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + expect(err).toBeNull(); + expect(stdout).toBe("I'm alive!"); + expect(stderr.trim()) + .toBe(`This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason: +test rejection`); + done(); + }); + })); + + test('should show error-type promise rejection warnings by default', () => + new Promise(done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'mode-warn-error.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + expect(err).toBeNull(); + expect(stdout).toBe("I'm alive!"); + expect(stderr) + .toContain(`This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason: +Error: test rejection + at Object.`); + done(); + }); + })); + + test('should not close process on unhandled rejection in strict mode', () => + new Promise(done => { + expect.assertions(4); + + const testScriptPath = path.resolve(__dirname, 'mode-strict.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + expect(err).not.toBeNull(); + expect(err?.code).toBe(1); + expect(stdout).not.toBe("I'm alive!"); + expect(stderr) + .toContain(`This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason: +test rejection`); + done(); + }); + })); + + test('should not close process or warn on unhandled rejection in none mode', () => + new Promise(done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'mode-none.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + expect(err).toBeNull(); + expect(stdout).toBe("I'm alive!"); + expect(stderr).toBe(''); + done(); + }); + })); + + test('captures exceptions for unhandled rejections', async () => { + await createRunner(__dirname, 'scenario-warn.ts') + .expect({ + event: { + level: 'error', + exception: { + values: [ + { + type: 'Error', + value: 'test rejection', + mechanism: { + type: 'onunhandledrejection', + handled: false, + }, + stacktrace: { + frames: expect.any(Array), + }, + }, + ], + }, + }, + }) + .start() + .completed(); + }); + + test('captures exceptions for unhandled rejections in strict mode', async () => { + await createRunner(__dirname, 'scenario-strict.ts') + .expect({ + event: { + level: 'fatal', + exception: { + values: [ + { + type: 'Error', + value: 'test rejection', + mechanism: { + type: 'onunhandledrejection', + handled: false, + }, + stacktrace: { + frames: expect.any(Array), + }, + }, + ], + }, + }, + }) + .start() + .completed(); + }); +}); diff --git a/packages/node/src/integrations/onunhandledrejection.ts b/packages/node/src/integrations/onunhandledrejection.ts index 4a7c7a1fe83d..8b41da189a0f 100644 --- a/packages/node/src/integrations/onunhandledrejection.ts +++ b/packages/node/src/integrations/onunhandledrejection.ts @@ -1,4 +1,4 @@ -import type { Client, IntegrationFn } from '@sentry/core'; +import type { Client, IntegrationFn, SeverityLevel } from '@sentry/core'; import { captureException, consoleSandbox, defineIntegration, getClient } from '@sentry/core'; import { logAndExitProcess } from '../utils/errorhandling'; @@ -15,12 +15,15 @@ interface OnUnhandledRejectionOptions { const INTEGRATION_NAME = 'OnUnhandledRejection'; const _onUnhandledRejectionIntegration = ((options: Partial = {}) => { - const mode = options.mode || 'warn'; + const opts = { + mode: 'warn', + ...options, + } satisfies OnUnhandledRejectionOptions; return { name: INTEGRATION_NAME, setup(client) { - global.process.on('unhandledRejection', makeUnhandledPromiseHandler(client, { mode })); + global.process.on('unhandledRejection', makeUnhandledPromiseHandler(client, opts)); }, }; }) satisfies IntegrationFn; @@ -46,10 +49,13 @@ export function makeUnhandledPromiseHandler( return; } + const level: SeverityLevel = options.mode === 'strict' ? 'fatal' : 'error'; + captureException(reason, { originalException: promise, captureContext: { extra: { unhandledPromiseRejection: true }, + level, }, mechanism: { handled: false, @@ -57,14 +63,14 @@ export function makeUnhandledPromiseHandler( }, }); - handleRejection(reason, options); + handleRejection(reason, options.mode); }; } /** * Handler for `mode` option */ -function handleRejection(reason: unknown, options: OnUnhandledRejectionOptions): void { +function handleRejection(reason: unknown, mode: UnhandledRejectionMode): void { // https://github.com/nodejs/node/blob/7cf6f9e964aa00772965391c23acda6d71972a9a/lib/internal/process/promises.js#L234-L240 const rejectionWarning = 'This error originated either by ' + @@ -73,12 +79,12 @@ function handleRejection(reason: unknown, options: OnUnhandledRejectionOptions): ' The promise rejected with the reason:'; /* eslint-disable no-console */ - if (options.mode === 'warn') { + if (mode === 'warn') { consoleSandbox(() => { console.warn(rejectionWarning); console.error(reason && typeof reason === 'object' && 'stack' in reason ? reason.stack : reason); }); - } else if (options.mode === 'strict') { + } else if (mode === 'strict') { consoleSandbox(() => { console.warn(rejectionWarning); }); From f00790acbc75b54b50710f8bed4916a61114f838 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Tue, 18 Mar 2025 14:12:36 +0100 Subject: [PATCH 2/3] Potential fix for code scanning alert no. 374: Shell command built from environment values Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../public-api/onUnhandledRejectionIntegration/test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts index 4c99f1b02052..bc0227990b7e 100644 --- a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts @@ -15,7 +15,7 @@ describe('onUnhandledRejectionIntegration', () => { const testScriptPath = path.resolve(__dirname, 'mode-warn-string.js'); - childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + childProcess.execFile('node', [testScriptPath], { encoding: 'utf8' }, (err, stdout, stderr) => { expect(err).toBeNull(); expect(stdout).toBe("I'm alive!"); expect(stderr.trim()) @@ -31,7 +31,7 @@ test rejection`); const testScriptPath = path.resolve(__dirname, 'mode-warn-error.js'); - childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + childProcess.execFile('node', [testScriptPath], { encoding: 'utf8' }, (err, stdout, stderr) => { expect(err).toBeNull(); expect(stdout).toBe("I'm alive!"); expect(stderr) @@ -48,7 +48,7 @@ Error: test rejection const testScriptPath = path.resolve(__dirname, 'mode-strict.js'); - childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + childProcess.execFile('node', [testScriptPath], { encoding: 'utf8' }, (err, stdout, stderr) => { expect(err).not.toBeNull(); expect(err?.code).toBe(1); expect(stdout).not.toBe("I'm alive!"); From c063a0ce5c4718257d13d4c904288c54b342ab41 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 18 Mar 2025 14:14:03 +0100 Subject: [PATCH 3/3] remaining --- .../suites/public-api/onUnhandledRejectionIntegration/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts index bc0227990b7e..e2f2a2143438 100644 --- a/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/onUnhandledRejectionIntegration/test.ts @@ -65,7 +65,7 @@ test rejection`); const testScriptPath = path.resolve(__dirname, 'mode-none.js'); - childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout, stderr) => { + childProcess.execFile('node', [testScriptPath], { encoding: 'utf8' }, (err, stdout, stderr) => { expect(err).toBeNull(); expect(stdout).toBe("I'm alive!"); expect(stderr).toBe('');