From a97e2b823efb402d62ecf1552358623e4968b861 Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Wed, 22 Nov 2023 18:46:50 -0800 Subject: [PATCH] Added error handling to Sentry's beforeSend (#19109) refs TryGhost/Product#4175 - Added error handling to Sentry's beforeSend function in both Admin and Core, so if there is any error in beforeSend, we will still send the unmodified event to Sentry - This is in response to an incident yesterday wherein the beforeSend function threw an error due to an unexpected missing value in the exception. The event sent to Sentry was the error in the beforeSend function, and the original error never reached Sentry. - If the original event had reached Sentry, even if unmodified by the logic in beforeSend, we could have been alerted to the issue sooner and more easily identified all affected sites. - Also added defensive logic to protect for certain values in the exception passed to beforeSend not existing and added unit tests for the beforeSend function in admin and core --- ghost/admin/app/routes/application.js | 35 +----- ghost/admin/app/utils/sentry.js | 43 +++++++ ghost/admin/tests/unit/utils/sentry-test.js | 117 ++++++++++++++++++++ ghost/core/core/shared/sentry.js | 89 +++++++++------ ghost/core/test/unit/shared/sentry.test.js | 89 +++++++++++++++ 5 files changed, 307 insertions(+), 66 deletions(-) create mode 100644 ghost/admin/app/utils/sentry.js create mode 100644 ghost/admin/tests/unit/utils/sentry-test.js diff --git a/ghost/admin/app/routes/application.js b/ghost/admin/app/routes/application.js index 7403f0facd..9973fd787e 100644 --- a/ghost/admin/app/routes/application.js +++ b/ghost/admin/app/routes/application.js @@ -8,6 +8,7 @@ import ShortcutsRoute from 'ghost-admin/mixins/shortcuts-route'; import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd'; import windowProxy from 'ghost-admin/utils/window-proxy'; import {Debug} from '@sentry/integrations'; +import {beforeSend} from 'ghost-admin/utils/sentry'; import {importComponent} from '../components/admin-x/admin-x-component'; import {inject} from 'ghost-admin/decorators/inject'; import { @@ -181,39 +182,7 @@ export default Route.extend(ShortcutsRoute, { dsn: this.config.sentry_dsn, environment: this.config.sentry_env, release: `ghost@${this.config.version}`, - beforeSend(event, hint) { - const exception = hint.originalException; - event.tags = event.tags || {}; - event.tags.shown_to_user = event.tags.shown_to_user || false; - event.tags.grammarly = !!document.querySelector('[data-gr-ext-installed]'); - - // Do not report "handled" errors to Sentry - if (event.tags.shown_to_user === true) { - return null; - } - - // if the error value includes a model id then overwrite it to improve grouping - if (event.exception.values && event.exception.values.length > 0) { - const pattern = /<(post|page):[a-f0-9]+>/; - const replacement = '<$1:ID>'; - event.exception.values[0].value = event.exception.values[0].value.replace(pattern, replacement); - } - - // ajax errors — improve logging and add context for debugging - if (isAjaxError(exception)) { - const error = exception.payload.errors[0]; - event.exception.values[0].type = `${error.type}: ${error.context}`; - event.exception.values[0].value = error.message; - event.exception.values[0].context = error.context; - } else { - delete event.contexts.ajax; - delete event.tags.ajax_status; - delete event.tags.ajax_method; - delete event.tags.ajax_url; - } - - return event; - }, + beforeSend, // TransitionAborted errors surface from normal application behaviour // - https://github.com/emberjs/ember.js/issues/12505 ignoreErrors: [/^TransitionAborted$/] diff --git a/ghost/admin/app/utils/sentry.js b/ghost/admin/app/utils/sentry.js new file mode 100644 index 0000000000..89b864cf86 --- /dev/null +++ b/ghost/admin/app/utils/sentry.js @@ -0,0 +1,43 @@ +import { + isAjaxError +} from 'ember-ajax/errors'; + +export function beforeSend(event, hint) { + try { + const exception = hint.originalException; + event.tags = event.tags || {}; + event.tags.shown_to_user = event.tags.shown_to_user || false; + event.tags.grammarly = !!document.querySelector('[data-gr-ext-installed]'); + + // Do not report "handled" errors to Sentry + if (event.tags.shown_to_user === true) { + return null; + } + + // if the error value includes a model id then overwrite it to improve grouping + if (event.exception && event.exception.values && event.exception.values.length > 0) { + const pattern = /<(post|page):[a-f0-9]+>/; + const replacement = '<$1:ID>'; + event.exception.values[0].value = event.exception.values[0].value.replace(pattern, replacement); + } + + // ajax errors — improve logging and add context for debugging + if (isAjaxError(exception) && exception.payload && exception.payload.errors && exception.payload.errors.length > 0) { + const error = exception.payload.errors[0]; + event.exception.values[0].type = `${error.type}: ${error.context}`; + event.exception.values[0].value = error.message; + event.exception.values[0].context = error.context; + } else { + delete event.contexts.ajax; + delete event.tags.ajax_status; + delete event.tags.ajax_method; + delete event.tags.ajax_url; + } + + return event; + } catch (error) { + // If any errors occur in beforeSend, send the original event to Sentry + // Better to have some information than no information + return event; + } +} \ No newline at end of file diff --git a/ghost/admin/tests/unit/utils/sentry-test.js b/ghost/admin/tests/unit/utils/sentry-test.js new file mode 100644 index 0000000000..709d4be872 --- /dev/null +++ b/ghost/admin/tests/unit/utils/sentry-test.js @@ -0,0 +1,117 @@ +import {beforeSend} from 'ghost-admin/utils/sentry'; +import {describe, it} from 'mocha'; +import {expect} from 'chai'; + +import * as emberErrors from 'ember-ajax/errors'; +import sinon from 'sinon'; + +describe('Unit: Util: sentry', function () { + let isAjaxErrorStub; + describe('beforeSend', function () { + before(function () { + isAjaxErrorStub = sinon.stub(emberErrors, 'isAjaxError'); + }); + + it('should return an event', () => { + isAjaxErrorStub.returns(false); + const event = { + test: 'test' + }; + const hint = {}; + + const result = beforeSend(event, hint); + expect(result).to.deep.equal(event); + }); + + it('does not send the event if it was shown to the user', () => { + isAjaxErrorStub.returns(false); + const event = { + tags: { + shown_to_user: true + } + }; + const hint = {}; + + const result = beforeSend(event, hint); + expect(result).to.equal(null); + }); + + it('removes post and page ids from the error message', () => { + isAjaxErrorStub.returns(false); + const event = { + exception: { + values: [ + { + value: 'Something went wrong ' + } + ] + } + }; + const hint = {}; + + const result = beforeSend(event, hint); + expect(result.exception.values[0].value).to.equal('Something went wrong '); + }); + + it('returns the original event if there is an error', () => { + isAjaxErrorStub.throws(new Error('test')); + + const event = { + test: 'test' + }; + const hint = {}; + + const result = beforeSend(event, hint); + expect(result).to.deep.equal(event); + }); + + it('returns the event even if the ajax error is missing values', () => { + isAjaxErrorStub.returns(true); + + const event = { + exception: { + values: [] + } + }; + const exception = { + payload: { + errors: [] + } + }; + const hint = { + originalException: exception + }; + + const result = beforeSend(event, hint); + expect(result).to.deep.equal(event); + }); + + it('removes ajax tags and context if it is not an ajax error', () => { + isAjaxErrorStub.returns(false); + + const event = { + tags: { + ajax_status: 'test status', + ajax_method: 'test method', + ajax_url: 'test url' + }, + contexts: { + ajax: 'test context' + } + }; + const hint = { + originalException: { + payload: { + errors: [] + } + } + }; + + const result = beforeSend(event, hint); + expect(result.tags.ajax_status).to.equal(undefined); + expect(result.tags.ajax_method).to.equal(undefined); + expect(result.tags.ajax_url).to.equal(undefined); + expect(result.contexts.ajax).to.equal(undefined); + }); + }); +}); diff --git a/ghost/core/core/shared/sentry.js b/ghost/core/core/shared/sentry.js index c2d52b10d7..aae14d5a93 100644 --- a/ghost/core/core/shared/sentry.js +++ b/ghost/core/core/shared/sentry.js @@ -2,6 +2,59 @@ const config = require('./config'); const sentryConfig = config.get('sentry'); const errors = require('@tryghost/errors'); +const beforeSend = function (event, hint) { + try { + const exception = hint.originalException; + const code = (exception && exception.code) ? exception.code : null; + const context = (exception && exception.context) ? exception.context : null; + const errorType = (exception && exception.errorType) ? exception.errorType : null; + const id = (exception && exception.id) ? exception.id : null; + const statusCode = (exception && exception.statusCode) ? exception.statusCode : null; + event.tags = event.tags || {}; + + if (errors.utils.isGhostError(exception)) { + // Unexpected errors have a generic error message, set it back to context if there is one + if (code === 'UNEXPECTED_ERROR' && context !== null) { + if (event.exception.values && event.exception.values.length > 0) { + event.exception.values[0].type = context; + } + } + + // This is a mysql2 error — add some additional context + if (exception.sql) { + const sql = exception.sql; + const errno = exception.errno ? exception.errno : null; + const sqlErrorCode = exception.sqlErrorCode ? exception.sqlErrorCode : null; + const sqlMessage = exception.sqlMessage ? exception.sqlMessage : null; + const sqlState = exception.sqlState ? exception.sqlState : null; + if (event.exception.values && event.exception.values.length > 0) { + event.exception.values[0].type = `SQL Error ${errno}: ${sqlErrorCode}`; + event.exception.values[0].value = sqlMessage; + event.contexts = event.contexts || {}; + event.contexts.mysql = { + errno: errno, + code: sqlErrorCode, + sql: sql, + message: sqlMessage, + state: sqlState + }; + } + } + + // This is a Ghost Error, copy all our extra data to tags + event.tags.type = errorType; + event.tags.code = code; + event.tags.id = id; + event.tags.status_code = statusCode; + } + return event; + } catch (error) { + // If any errors occur in beforeSend, send the original event to Sentry + // Better to have some information than no information + return event; + } +}; + if (sentryConfig && !sentryConfig.disabled) { const Sentry = require('@sentry/node'); const version = require('@tryghost/version').full; @@ -11,38 +64,7 @@ if (sentryConfig && !sentryConfig.disabled) { release: 'ghost@' + version, environment: environment, maxValueLength: 1000, - beforeSend: function (event, hint) { - const exception = hint.originalException; - - event.tags = event.tags || {}; - - if (errors.utils.isGhostError(exception)) { - // Unexpected errors have a generic error message, set it back to context if there is one - if (exception.code === 'UNEXPECTED_ERROR' && exception.context !== null) { - event.exception.values[0].type = exception.context; - } - - // This is a mysql2 error — add some additional context - if (exception.sql) { - event.exception.values[0].type = `SQL Error ${exception.errno}: ${exception.sqlErrorCode}`; - event.exception.values[0].value = exception.sqlMessage; - event.contexts.mysql = { - errno: exception.errno, - code: exception.sqlErrorCode, - sql: exception.sql, - message: exception.sqlMessage, - state: exception.sqlState - }; - } - - // This is a Ghost Error, copy all our extra data to tags - event.tags.type = exception.errorType; - event.tags.code = exception.code; - event.tags.id = exception.id; - event.tags.status_code = exception.statusCode; - } - return event; - } + beforeSend: beforeSend }); module.exports = { @@ -60,7 +82,8 @@ if (sentryConfig && !sentryConfig.disabled) { return (error.statusCode === 500); } }), - captureException: Sentry.captureException + captureException: Sentry.captureException, + beforeSend: beforeSend }; } else { const expressNoop = function (req, res, next) { diff --git a/ghost/core/test/unit/shared/sentry.test.js b/ghost/core/test/unit/shared/sentry.test.js index cdd219b3a1..135727413d 100644 --- a/ghost/core/test/unit/shared/sentry.test.js +++ b/ghost/core/test/unit/shared/sentry.test.js @@ -1,6 +1,7 @@ const assert = require('assert/strict'); const sinon = require('sinon'); const configUtils = require('../../utils/configUtils'); +const errors = require('@tryghost/errors'); const Sentry = require('@sentry/node'); @@ -51,4 +52,92 @@ describe('UNIT: sentry', function () { assert.ok(initArgs[0].hasOwnProperty('beforeSend'), 'should have a beforeSend function'); }); }); + + describe('beforeSend', function () { + this.beforeEach(function () { + configUtils.set({sentry: {disabled: false, dsn: fakeDSN}}); + delete require.cache[require.resolve('../../../core/shared/sentry')]; + + sentry = require('../../../core/shared/sentry'); + }); + + it('returns the event', function () { + sinon.stub(errors.utils, 'isGhostError').returns(false); + const beforeSend = sentry.beforeSend; + const event = {tags: {}}; + const hint = {}; + + const result = beforeSend(event, hint); + + assert.deepEqual(result, event); + }); + + it('returns the event, even if an exception is thrown internally', function () { + // Trigger an internal exception + sinon.stub(errors.utils, 'isGhostError').throws(new Error('test')); + const beforeSend = sentry.beforeSend; + const event = {tags: {}}; + const hint = {}; + + const result = beforeSend(event, hint); + + assert.deepEqual(result, event); + }); + + it('sets sql context for mysql2 errors', function () { + sinon.stub(errors.utils, 'isGhostError').returns(true); + const beforeSend = sentry.beforeSend; + const event = { + tags: {}, + exception: { + values: [{ + value: 'test', + type: 'test' + }] + } + }; + const exception = { + sql: 'SELECT * FROM test', + errno: 123, + sqlErrorCode: 456, + sqlMessage: 'test message', + sqlState: 'test state', + code: 'UNEXPECTED_ERROR', + errorType: 'InternalServerError', + id: 'a1b2c3d4e5f6', + statusCode: 500 + }; + const hint = { + originalException: exception + }; + + const result = beforeSend(event, hint); + + const expected = { + tags: { + type: 'InternalServerError', + code: 'UNEXPECTED_ERROR', + id: 'a1b2c3d4e5f6', + status_code: 500 + }, + exception: { + values: [{ + value: 'test message', + type: 'SQL Error 123: 456' + }] + }, + contexts: { + mysql: { + errno: 123, + code: 456, + sql: 'SELECT * FROM test', + message: 'test message', + state: 'test state' + } + } + }; + + assert.deepEqual(result, expected); + }); + }); });