From 32ea4ac237795d7862f5a8dc39548ed05c5eccee Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Thu, 9 Jan 2025 16:14:45 +0200 Subject: [PATCH] fix(core): Filter out task runner errors originating from user code from sentry (no-changelog) --- .../src/__tests__/task-runner-sentry.test.ts | 178 ++++++++++++++++++ packages/@n8n/task-runner/src/start.ts | 26 +-- .../task-runner/src/task-runner-sentry.ts | 63 +++++++ packages/core/src/error-reporter.ts | 68 +++++-- packages/core/test/error-reporter.test.ts | 31 +++ 5 files changed, 330 insertions(+), 36 deletions(-) create mode 100644 packages/@n8n/task-runner/src/__tests__/task-runner-sentry.test.ts create mode 100644 packages/@n8n/task-runner/src/task-runner-sentry.ts diff --git a/packages/@n8n/task-runner/src/__tests__/task-runner-sentry.test.ts b/packages/@n8n/task-runner/src/__tests__/task-runner-sentry.test.ts new file mode 100644 index 0000000000000..6fe14a484ad50 --- /dev/null +++ b/packages/@n8n/task-runner/src/__tests__/task-runner-sentry.test.ts @@ -0,0 +1,178 @@ +import type { ErrorEvent } from '@sentry/types'; +import { mock } from 'jest-mock-extended'; +import type { ErrorReporter } from 'n8n-core'; + +import { TaskRunnerSentry } from '../task-runner-sentry'; + +describe('TaskRunnerSentry', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + describe('filterOutUserCodeErrors', () => { + const sentry = new TaskRunnerSentry( + { + dsn: 'https://sentry.io/123', + n8nVersion: '1.0.0', + environment: 'local', + deploymentName: 'test', + }, + mock(), + ); + + it('should filter out user code errors', () => { + const event: ErrorEvent = { + type: undefined, + exception: { + values: [ + { + type: 'ReferenceError', + value: 'fetch is not defined', + stacktrace: { + frames: [ + { + filename: 'app:///dist/js-task-runner/js-task-runner.js', + module: 'js-task-runner:js-task-runner', + function: 'JsTaskRunner.executeTask', + }, + { + filename: 'app:///dist/js-task-runner/js-task-runner.js', + module: 'js-task-runner:js-task-runner', + function: 'JsTaskRunner.runForAllItems', + }, + { + filename: '', + module: '', + function: 'new Promise', + }, + { + filename: 'app:///dist/js-task-runner/js-task-runner.js', + module: 'js-task-runner:js-task-runner', + function: 'result', + }, + { + filename: 'node:vm', + module: 'node:vm', + function: 'runInContext', + }, + { + filename: 'node:vm', + module: 'node:vm', + function: 'Script.runInContext', + }, + { + filename: 'evalmachine.', + module: 'evalmachine.', + function: '?', + }, + { + filename: 'evalmachine.', + module: 'evalmachine.', + function: 'VmCodeWrapper', + }, + { + filename: '', + module: '', + function: 'new Promise', + }, + { + filename: 'evalmachine.', + module: 'evalmachine.', + }, + ], + }, + mechanism: { type: 'onunhandledrejection', handled: false }, + }, + ], + }, + event_id: '18bb78bb3d9d44c4acf3d774c2cfbfd8', + platform: 'node', + contexts: { + trace: { trace_id: '3c3614d33a6b47f09b85ec7d2710acea', span_id: 'ad00fdf6d6173aeb' }, + runtime: { name: 'node', version: 'v20.17.0' }, + }, + }; + + expect(sentry.filterOutUserCodeErrors(event)).toBe(true); + }); + }); + + describe('initIfEnabled', () => { + const mockErrorReporter = mock(); + + it('should not configure sentry if dsn is not set', async () => { + const sentry = new TaskRunnerSentry( + { + dsn: '', + n8nVersion: '1.0.0', + environment: 'local', + deploymentName: 'test', + }, + mockErrorReporter, + ); + + await sentry.initIfEnabled(); + + expect(mockErrorReporter.init).not.toHaveBeenCalled(); + }); + + it('should configure sentry if dsn is set', async () => { + const sentry = new TaskRunnerSentry( + { + dsn: 'https://sentry.io/123', + n8nVersion: '1.0.0', + environment: 'local', + deploymentName: 'test', + }, + mockErrorReporter, + ); + + await sentry.initIfEnabled(); + + expect(mockErrorReporter.init).toHaveBeenCalledWith({ + dsn: 'https://sentry.io/123', + beforeSendFilter: sentry.filterOutUserCodeErrors, + release: '1.0.0', + environment: 'local', + serverName: 'test', + serverType: 'task_runner', + }); + }); + }); + + describe('shutdown', () => { + const mockErrorReporter = mock(); + + it('should not shutdown sentry if dsn is not set', async () => { + const sentry = new TaskRunnerSentry( + { + dsn: '', + n8nVersion: '1.0.0', + environment: 'local', + deploymentName: 'test', + }, + mockErrorReporter, + ); + + await sentry.shutdown(); + + expect(mockErrorReporter.shutdown).not.toHaveBeenCalled(); + }); + + it('should shutdown sentry if dsn is set', async () => { + const sentry = new TaskRunnerSentry( + { + dsn: 'https://sentry.io/123', + n8nVersion: '1.0.0', + environment: 'local', + deploymentName: 'test', + }, + mockErrorReporter, + ); + + await sentry.shutdown(); + + expect(mockErrorReporter.shutdown).toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/@n8n/task-runner/src/start.ts b/packages/@n8n/task-runner/src/start.ts index cbc381c2fd75f..35b928e9a85b0 100644 --- a/packages/@n8n/task-runner/src/start.ts +++ b/packages/@n8n/task-runner/src/start.ts @@ -1,16 +1,16 @@ import './polyfills'; import { Container } from '@n8n/di'; -import type { ErrorReporter } from 'n8n-core'; import { ensureError, setGlobalState } from 'n8n-workflow'; import { MainConfig } from './config/main-config'; import type { HealthCheckServer } from './health-check-server'; import { JsTaskRunner } from './js-task-runner/js-task-runner'; +import { TaskRunnerSentry } from './task-runner-sentry'; let healthCheckServer: HealthCheckServer | undefined; let runner: JsTaskRunner | undefined; let isShuttingDown = false; -let errorReporter: ErrorReporter | undefined; +let sentry: TaskRunnerSentry | undefined; function createSignalHandler(signal: string, timeoutInS = 10) { return async function onSignal() { @@ -33,9 +33,9 @@ function createSignalHandler(signal: string, timeoutInS = 10) { void healthCheckServer?.stop(); } - if (errorReporter) { - await errorReporter.shutdown(); - errorReporter = undefined; + if (sentry) { + await sentry.shutdown(); + sentry = undefined; } } catch (e) { const error = ensureError(e); @@ -54,20 +54,8 @@ void (async function start() { defaultTimezone: config.baseRunnerConfig.timezone, }); - const { dsn } = config.sentryConfig; - - if (dsn) { - const { ErrorReporter } = await import('n8n-core'); - errorReporter = Container.get(ErrorReporter); - const { deploymentName, environment, n8nVersion } = config.sentryConfig; - await errorReporter.init({ - serverType: 'task_runner', - dsn, - serverName: deploymentName, - environment, - release: n8nVersion, - }); - } + sentry = Container.get(TaskRunnerSentry); + await sentry.initIfEnabled(); runner = new JsTaskRunner(config); runner.on('runner:reached-idle-timeout', () => { diff --git a/packages/@n8n/task-runner/src/task-runner-sentry.ts b/packages/@n8n/task-runner/src/task-runner-sentry.ts new file mode 100644 index 0000000000000..d532b9c7e6f05 --- /dev/null +++ b/packages/@n8n/task-runner/src/task-runner-sentry.ts @@ -0,0 +1,63 @@ +import { Service } from '@n8n/di'; +import type { ErrorEvent, Exception } from '@sentry/types'; +import { ErrorReporter } from 'n8n-core'; + +import { SentryConfig } from './config/sentry-config'; + +/** + * Sentry service for the task runner. + */ +@Service() +export class TaskRunnerSentry { + constructor( + private readonly config: SentryConfig, + private readonly errorReporter: ErrorReporter, + ) {} + + async initIfEnabled() { + if (!this.config.dsn) return; + + await this.errorReporter.init({ + serverType: 'task_runner', + dsn: this.config.dsn, + release: this.config.n8nVersion, + environment: this.config.environment, + serverName: this.config.deploymentName, + beforeSendFilter: this.filterOutUserCodeErrors, + }); + } + + async shutdown() { + if (!this.config.dsn) return; + + await this.errorReporter.shutdown(); + } + + /** + * Filter out errors originating from user provided code. + * It is possible for users to create code that causes unhandledrejections + * that end up in the sentry error reporting. + */ + filterOutUserCodeErrors = (event: ErrorEvent) => { + const error = event?.exception?.values?.[0]; + if (!error) return false; + + return error ? this.isUserCodeError(error) : false; + }; + + /** + * Check if the error is originating from user provided code. + * It is possible for users to create code that causes unhandledrejections + * that end up in the sentry error reporting. + */ + private isUserCodeError(error?: Exception) { + if (!error) return false; + + const frames = error.stacktrace?.frames; + if (!frames) return false; + + return frames.some( + (frame) => frame.filename === 'node:vm' && frame.function === 'runInContext', + ); + } +} diff --git a/packages/core/src/error-reporter.ts b/packages/core/src/error-reporter.ts index 84f67a03959d6..d6f2c858b07a7 100644 --- a/packages/core/src/error-reporter.ts +++ b/packages/core/src/error-reporter.ts @@ -15,6 +15,11 @@ type ErrorReporterInitOptions = { release: string; environment: string; serverName: string; + /** + * Function to allow filtering out errors before they are sent to Sentry. + * Return true if the error should be filtered out. + */ + beforeSendFilter?: (event: ErrorEvent, hint: EventHint) => boolean; }; @Service() @@ -24,6 +29,8 @@ export class ErrorReporter { private report: (error: Error | string, options?: ReportingOptions) => void; + private beforeSendFilter?: (event: ErrorEvent, hint: EventHint) => boolean; + constructor(private readonly logger: Logger) { // eslint-disable-next-line @typescript-eslint/unbound-method this.report = this.defaultReport; @@ -52,7 +59,14 @@ export class ErrorReporter { await close(timeoutInMs); } - async init({ dsn, serverType, release, environment, serverName }: ErrorReporterInitOptions) { + async init({ + beforeSendFilter, + dsn, + serverType, + release, + environment, + serverName, + }: ErrorReporterInitOptions) { process.on('uncaughtException', (error) => { this.error(error); }); @@ -100,9 +114,16 @@ export class ErrorReporter { setTag('server_type', serverType); this.report = (error, options) => captureException(error, options); + this.beforeSendFilter = beforeSendFilter; } - async beforeSend(event: ErrorEvent, { originalException }: EventHint) { + async beforeSend(event: ErrorEvent, hint: EventHint) { + if (this.beforeSendFilter && this.beforeSendFilter(event, hint)) { + return null; + } + + let { originalException } = hint; + if (!originalException) return null; if (originalException instanceof Promise) { @@ -111,21 +132,8 @@ export class ErrorReporter { if (originalException instanceof AxiosError) return null; - if ( - originalException instanceof Error && - originalException.name === 'QueryFailedError' && - ['SQLITE_FULL', 'SQLITE_IOERR'].some((errMsg) => originalException.message.includes(errMsg)) - ) { - return null; - } - - if (originalException instanceof ApplicationError) { - const { level, extra, tags } = originalException; - if (level === 'warning') return null; - event.level = level; - if (extra) event.extra = { ...event.extra, ...extra }; - if (tags) event.tags = { ...event.tags, ...tags }; - } + if (this.handleSqliteError(originalException)) return null; + if (this.handleApplicationError(event, originalException)) return null; if ( originalException instanceof Error && @@ -166,4 +174,30 @@ export class ErrorReporter { if (typeof e === 'string') return new ApplicationError(e); return; } + + /** @returns true if the error should be filtered out */ + private handleSqliteError(error: unknown) { + if ( + error instanceof Error && + error.name === 'QueryFailedError' && + ['SQLITE_FULL', 'SQLITE_IOERR'].some((errMsg) => error.message.includes(errMsg)) + ) { + return true; + } + + return false; + } + + /** @returns true if the error should be filtered out */ + private handleApplicationError(event: ErrorEvent, originalException: unknown) { + if (originalException instanceof ApplicationError) { + const { level, extra, tags } = originalException; + if (level === 'warning') return true; + event.level = level; + if (extra) event.extra = { ...event.extra, ...extra }; + if (tags) event.tags = { ...event.tags, ...tags }; + } + + return false; + } } diff --git a/packages/core/test/error-reporter.test.ts b/packages/core/test/error-reporter.test.ts index 9edc27f15ccd9..e5b51d3356b39 100644 --- a/packages/core/test/error-reporter.test.ts +++ b/packages/core/test/error-reporter.test.ts @@ -101,6 +101,37 @@ describe('ErrorReporter', () => { const result = await errorReporter.beforeSend(event, { originalException }); expect(result).toBeNull(); }); + + describe('beforeSendFilter', () => { + const newErrorReportedWithBeforeSendFilter = (beforeSendFilter: jest.Mock) => { + const errorReporter = new ErrorReporter(mock()); + // @ts-expect-error - beforeSendFilter is private + errorReporter.beforeSendFilter = beforeSendFilter; + return errorReporter; + }; + + it('should filter out based on the beforeSendFilter', async () => { + const beforeSendFilter = jest.fn().mockReturnValue(true); + const errorReporter = newErrorReportedWithBeforeSendFilter(beforeSendFilter); + const hint = { originalException: new Error() }; + + const result = await errorReporter.beforeSend(event, hint); + + expect(result).toBeNull(); + expect(beforeSendFilter).toHaveBeenCalledWith(event, hint); + }); + + it('should not filter out when beforeSendFilter returns false', async () => { + const beforeSendFilter = jest.fn().mockReturnValue(false); + const errorReporter = newErrorReportedWithBeforeSendFilter(beforeSendFilter); + const hint = { originalException: new Error() }; + + const result = await errorReporter.beforeSend(event, hint); + + expect(result).toEqual(event); + expect(beforeSendFilter).toHaveBeenCalledWith(event, hint); + }); + }); }); describe('error', () => {