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

@@ -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$/]

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;
+    }
+}

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 <post:123>'
+                        }
+                    ]
+                }
+            };
+            const hint = {};
+
+            const result = beforeSend(event, hint);
+            expect(result.exception.values[0].value).to.equal('Something went wrong <post:ID>');
+        });
+
+        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);
+        });
+    });
+});

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) {

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);
+        });
+    });
 });