mirror of
https://github.com/TryGhost/Ghost.git
synced 2025-03-25 02:31:59 -05:00
Fixed raw JS error messages appearing in alerts (#2407)
refs https://github.com/TryGhost/Team/issues/1613 We use `notifications.showAPIError()` in many of our try/catch routines but those can also pick up standard JS errors which can result in ugly and useless messages showing in alerts. - added a list of known built-in JS error type names to check against and a generic error message to be used in place of ones we know shouldn't be displayed - in `showAPIError(obj)` check `obj.name` against the known list and swap the message for a generic one - only the message is swapped, we still log the full/original error to Sentry - in `handleNotification(msg)` which is the final method used when displaying any alert/notification, extract all words in the supplied message and check that against the known list and swap the message on a match. This handles situations where the API might give us a raw JS error message in the message string
This commit is contained in:
parent
b9707a2b4a
commit
c24ea5d0bc
2 changed files with 60 additions and 3 deletions
|
@ -20,6 +20,23 @@ import {tracked} from '@glimmer/tracking';
|
|||
// to avoid stacking of multiple error messages whilst leaving enough
|
||||
// specificity to re-use keys for i18n lookups
|
||||
|
||||
// Rather than showing raw JS error messages to users we want to show a generic one.
|
||||
// This list is used to check obj.name in `showApiError(obj)` as the first line
|
||||
// of defence, then at the lowest `handleNotification(msg)` level we check the
|
||||
// first word of the message text as a fallback in case we get JS error messages
|
||||
// from the API. If there's a match we show the generic message.
|
||||
const GENERIC_ERROR_NAMES = [
|
||||
'AggregateError',
|
||||
'EvalError',
|
||||
'RangeError',
|
||||
'ReferenceError',
|
||||
'SyntaxError',
|
||||
'TypeError',
|
||||
'URIError'
|
||||
];
|
||||
|
||||
export const GENERIC_ERROR_MESSAGE = 'An unexpected error occurred, please try again.';
|
||||
|
||||
export default class NotificationsService extends Service {
|
||||
@service config;
|
||||
@service upgradeStatus;
|
||||
|
@ -35,7 +52,17 @@ export default class NotificationsService extends Service {
|
|||
return this.content.filter(n => n.status === 'notification');
|
||||
}
|
||||
|
||||
handleNotification(message, delayed) {
|
||||
handleNotification(message, delayed = false) {
|
||||
const wordRegex = /[a-z]+/igm;
|
||||
const wordMatches = (message.message.string || message.message).matchAll(wordRegex);
|
||||
|
||||
for (const wordMatch of wordMatches) {
|
||||
if (GENERIC_ERROR_NAMES.includes(wordMatch[0])) {
|
||||
message.message = GENERIC_ERROR_MESSAGE;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
// If this is an alert message from the server, treat it as html safe
|
||||
if (message.constructor.modelName === 'notification' && message.status === 'alert') {
|
||||
message.message = htmlSafe(message.message);
|
||||
|
@ -124,7 +151,7 @@ export default class NotificationsService extends Service {
|
|||
}
|
||||
|
||||
// loop over ember-ajax errors object
|
||||
if (resp && resp.payload && isArray(resp.payload.errors)) {
|
||||
if (isArray(resp?.payload?.errors)) {
|
||||
return resp.payload.errors.forEach((error) => {
|
||||
this._showAPIError(error, options);
|
||||
});
|
||||
|
@ -147,7 +174,9 @@ export default class NotificationsService extends Service {
|
|||
|
||||
let msg = options.defaultErrorText || 'There was a problem on the server, please try again.';
|
||||
|
||||
if (resp instanceof String) {
|
||||
if (resp?.name && GENERIC_ERROR_NAMES.includes(resp.name)) {
|
||||
msg = GENERIC_ERROR_MESSAGE;
|
||||
} else if (resp instanceof String) {
|
||||
msg = resp;
|
||||
} else if (!isBlank(resp?.detail)) {
|
||||
msg = resp.detail;
|
||||
|
|
|
@ -8,6 +8,8 @@ import {expect} from 'chai';
|
|||
import {run} from '@ember/runloop';
|
||||
import {setupTest} from 'ember-mocha';
|
||||
|
||||
import {GENERIC_ERROR_MESSAGE} from 'ghost-admin/services/notifications';
|
||||
|
||||
// notifications service determines if a notification is a model instance by
|
||||
// checking `notification.constructor.modelName === 'notification'`
|
||||
const NotificationStub = EmberObject.extend();
|
||||
|
@ -58,6 +60,23 @@ describe('Unit: Service: notifications', function () {
|
|||
.to.deep.include({message: 'Test', status: 'notification'});
|
||||
});
|
||||
|
||||
it('#handleNotification shows generic error message when a word matches built-in error type', function () {
|
||||
let notifications = this.owner.lookup('service:notifications');
|
||||
|
||||
notifications.handleNotification({message: 'TypeError test'});
|
||||
expect(notifications.content[0].message).to.equal(GENERIC_ERROR_MESSAGE);
|
||||
|
||||
notifications.clearAll();
|
||||
expect(notifications.content.length).to.equal(0);
|
||||
|
||||
notifications.handleNotification({message: 'TypeError: Testing'});
|
||||
expect(notifications.content[0].message).to.equal(GENERIC_ERROR_MESSAGE);
|
||||
|
||||
notifications.clearAll();
|
||||
notifications.handleNotification({message: 'Unknown error - TypeError, cannot save invite.'});
|
||||
expect(notifications.content[0].message).to.equal(GENERIC_ERROR_MESSAGE);
|
||||
});
|
||||
|
||||
it('#showAlert adds POJO alerts', function () {
|
||||
let notifications = this.owner.lookup('service:notifications');
|
||||
|
||||
|
@ -255,6 +274,15 @@ describe('Unit: Service: notifications', function () {
|
|||
expect(alert.key).to.equal('api-error');
|
||||
});
|
||||
|
||||
it('#showAPIError shows generic error for built-in error types', function () {
|
||||
let notifications = this.owner.lookup('service:notifications');
|
||||
const error = new TypeError('Testing');
|
||||
|
||||
notifications.showAPIError(error);
|
||||
|
||||
expect(notifications.alerts[0].message).to.equal(GENERIC_ERROR_MESSAGE);
|
||||
});
|
||||
|
||||
it('#displayDelayed moves delayed notifications into content', function () {
|
||||
let notifications = this.owner.lookup('service:notifications');
|
||||
|
||||
|
|
Loading…
Add table
Reference in a new issue