Skip to content

Commit 672bfae

Browse files
GudahttGudahtt
committed
Protect against captureException error
1 parent b949b47 commit 672bfae

File tree

4 files changed

+220
-10
lines changed

4 files changed

+220
-10
lines changed

‎packages/base-controller/src/BaseController.test.ts‎

Lines changed: 148 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -796,7 +796,7 @@ describe('getAnonymizedState', () => {
796796

797797
it('reports thrown error when deriving state', () => {
798798
const captureException = jest.fn();
799-
const persistentState = getAnonymizedState(
799+
const anonymizedState = getAnonymizedState(
800800
{
801801
extraState: 'extraState',
802802
privateKey: '123',
@@ -820,7 +820,7 @@ describe('getAnonymizedState', () => {
820820
captureException,
821821
);
822822

823-
expect(persistentState).toStrictEqual({
823+
expect(anonymizedState).toStrictEqual({
824824
privateKey: '123',
825825
});
826826
expect(captureException).toHaveBeenCalledTimes(1);
@@ -829,11 +829,58 @@ describe('getAnonymizedState', () => {
829829
);
830830
});
831831

832+
it('logs thrown error and captureException error to console if captureException throws', () => {
833+
const consoleError = jest.fn();
834+
const testError = new Error('Test error');
835+
const captureException = jest.fn().mockImplementation(() => {
836+
throw testError;
837+
});
838+
jest.spyOn(console, 'error').mockImplementation(consoleError);
839+
const anonymizedState = getAnonymizedState(
840+
{
841+
extraState: 'extraState',
842+
privateKey: '123',
843+
network: 'mainnet',
844+
},
845+
// @ts-expect-error Intentionally testing invalid state
846+
{
847+
privateKey: {
848+
anonymous: true,
849+
includeInStateLogs: false,
850+
persist: false,
851+
usedInUi: false,
852+
},
853+
network: {
854+
anonymous: false,
855+
includeInStateLogs: false,
856+
persist: false,
857+
usedInUi: false,
858+
},
859+
},
860+
captureException,
861+
);
862+
863+
expect(anonymizedState).toStrictEqual({
864+
privateKey: '123',
865+
});
866+
867+
expect(consoleError).toHaveBeenCalledTimes(2);
868+
expect(consoleError).toHaveBeenNthCalledWith(
869+
1,
870+
new Error(`Error thrown when calling 'captureException'`),
871+
testError,
872+
);
873+
expect(consoleError).toHaveBeenNthCalledWith(
874+
2,
875+
new Error(`No metadata found for 'extraState'`),
876+
);
877+
});
878+
832879
it('logs thrown error to console when deriving state if no captureException function is given', () => {
833880
const consoleError = jest.fn();
834881
jest.spyOn(console, 'error').mockImplementation(consoleError);
835882

836-
const persistentState = getAnonymizedState(
883+
const anonymizedState = getAnonymizedState(
837884
{
838885
extraState: 'extraState',
839886
privateKey: '123',
@@ -856,7 +903,7 @@ describe('getAnonymizedState', () => {
856903
},
857904
);
858905

859-
expect(persistentState).toStrictEqual({
906+
expect(anonymizedState).toStrictEqual({
860907
privateKey: '123',
861908
});
862909
expect(consoleError).toHaveBeenCalledTimes(1);
@@ -1073,6 +1120,53 @@ describe('getPersistentState', () => {
10731120
);
10741121
});
10751122

1123+
it('logs thrown error and captureException error to console if captureException throws', () => {
1124+
const consoleError = jest.fn();
1125+
const testError = new Error('Test error');
1126+
const captureException = jest.fn().mockImplementation(() => {
1127+
throw testError;
1128+
});
1129+
jest.spyOn(console, 'error').mockImplementation(consoleError);
1130+
const persistentState = getPersistentState(
1131+
{
1132+
extraState: 'extraState',
1133+
privateKey: '123',
1134+
network: 'mainnet',
1135+
},
1136+
// @ts-expect-error Intentionally testing invalid state
1137+
{
1138+
privateKey: {
1139+
anonymous: false,
1140+
includeInStateLogs: false,
1141+
persist: true,
1142+
usedInUi: false,
1143+
},
1144+
network: {
1145+
anonymous: false,
1146+
includeInStateLogs: false,
1147+
persist: false,
1148+
usedInUi: false,
1149+
},
1150+
},
1151+
captureException,
1152+
);
1153+
1154+
expect(persistentState).toStrictEqual({
1155+
privateKey: '123',
1156+
});
1157+
1158+
expect(consoleError).toHaveBeenCalledTimes(2);
1159+
expect(consoleError).toHaveBeenNthCalledWith(
1160+
1,
1161+
new Error(`Error thrown when calling 'captureException'`),
1162+
testError,
1163+
);
1164+
expect(consoleError).toHaveBeenNthCalledWith(
1165+
2,
1166+
new Error(`No metadata found for 'extraState'`),
1167+
);
1168+
});
1169+
10761170
it('logs thrown error to console when deriving state if no captureException function is given', () => {
10771171
const consoleError = jest.fn();
10781172
jest.spyOn(console, 'error').mockImplementation(consoleError);
@@ -1411,6 +1505,56 @@ describe('deriveStateFromMetadata', () => {
14111505
expect(captureException).toHaveBeenCalledWith(new Error(testException));
14121506
});
14131507

1508+
it('logs thrown error and captureException error to console if captureException throws', () => {
1509+
const consoleError = jest.fn();
1510+
const testError = new Error('Test error');
1511+
const captureException = jest.fn().mockImplementation(() => {
1512+
throw testError;
1513+
});
1514+
jest.spyOn(console, 'error').mockImplementation(consoleError);
1515+
const derivedState = deriveStateFromMetadata(
1516+
{
1517+
extraState: 'extraState',
1518+
privateKey: '123',
1519+
network: 'mainnet',
1520+
},
1521+
// @ts-expect-error Intentionally testing invalid state
1522+
{
1523+
privateKey: {
1524+
anonymous: false,
1525+
includeInStateLogs: false,
1526+
persist: false,
1527+
usedInUi: false,
1528+
[property]: true,
1529+
},
1530+
network: {
1531+
anonymous: false,
1532+
includeInStateLogs: false,
1533+
persist: false,
1534+
usedInUi: false,
1535+
[property]: false,
1536+
},
1537+
},
1538+
property,
1539+
captureException,
1540+
);
1541+
1542+
expect(derivedState).toStrictEqual({
1543+
privateKey: '123',
1544+
});
1545+
1546+
expect(consoleError).toHaveBeenCalledTimes(2);
1547+
expect(consoleError).toHaveBeenNthCalledWith(
1548+
1,
1549+
new Error(`Error thrown when calling 'captureException'`),
1550+
testError,
1551+
);
1552+
expect(consoleError).toHaveBeenNthCalledWith(
1553+
2,
1554+
new Error(`No metadata found for 'extraState'`),
1555+
);
1556+
});
1557+
14141558
it('logs thrown error to console when deriving state if no captureException function is given', () => {
14151559
const consoleError = jest.fn();
14161560
jest.spyOn(console, 'error').mockImplementation(consoleError);

‎packages/base-controller/src/BaseController.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -425,9 +425,17 @@ export function deriveStateFromMetadata<
425425
// Capture error without interrupting state-related operations
426426
// See [ADR core#0016](https://github.com/MetaMask/decisions/blob/main/decisions/core/0016-core-classes-error-reporting.md)
427427
if (captureException) {
428-
captureException(
429-
error instanceof Error ? error : new Error(String(error)),
430-
);
428+
try {
429+
captureException(
430+
error instanceof Error ? error : new Error(String(error)),
431+
);
432+
} catch (captureExceptionError) {
433+
console.error(
434+
new Error(`Error thrown when calling 'captureException'`),
435+
captureExceptionError,
436+
);
437+
console.error(error);
438+
}
431439
} else {
432440
console.error(error);
433441
}

‎packages/base-controller/src/next/BaseController.test.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,6 +1077,56 @@ describe('deriveStateFromMetadata', () => {
10771077
expect(captureException).toHaveBeenCalledWith(new Error(testException));
10781078
});
10791079

1080+
it('logs thrown error and captureException error to console if captureException throws', () => {
1081+
const consoleError = jest.fn();
1082+
const testError = new Error('Test error');
1083+
const captureException = jest.fn().mockImplementation(() => {
1084+
throw testError;
1085+
});
1086+
jest.spyOn(console, 'error').mockImplementation(consoleError);
1087+
const derivedState = deriveStateFromMetadata(
1088+
{
1089+
extraState: 'extraState',
1090+
privateKey: '123',
1091+
network: 'mainnet',
1092+
},
1093+
// @ts-expect-error Intentionally testing invalid state
1094+
{
1095+
privateKey: {
1096+
includeInDebugSnapshot: false,
1097+
includeInStateLogs: false,
1098+
persist: false,
1099+
usedInUi: false,
1100+
[property]: true,
1101+
},
1102+
network: {
1103+
includeInDebugSnapshot: false,
1104+
includeInStateLogs: false,
1105+
persist: false,
1106+
usedInUi: false,
1107+
[property]: false,
1108+
},
1109+
},
1110+
property,
1111+
captureException,
1112+
);
1113+
1114+
expect(derivedState).toStrictEqual({
1115+
privateKey: '123',
1116+
});
1117+
1118+
expect(consoleError).toHaveBeenCalledTimes(2);
1119+
expect(consoleError).toHaveBeenNthCalledWith(
1120+
1,
1121+
new Error(`Error thrown when calling 'captureException'`),
1122+
testError,
1123+
);
1124+
expect(consoleError).toHaveBeenNthCalledWith(
1125+
2,
1126+
new Error(`No metadata found for 'extraState'`),
1127+
);
1128+
});
1129+
10801130
it('logs thrown error to console when deriving state if no captureException function is given', () => {
10811131
const consoleError = jest.fn();
10821132
jest.spyOn(console, 'error').mockImplementation(consoleError);

‎packages/base-controller/src/next/BaseController.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -398,9 +398,17 @@ export function deriveStateFromMetadata<
398398
// Capture error without interrupting state-related operations
399399
// See [ADR core#0016](https://github.com/MetaMask/decisions/blob/main/decisions/core/0016-core-classes-error-reporting.md)
400400
if (captureException) {
401-
captureException(
402-
error instanceof Error ? error : new Error(String(error)),
403-
);
401+
try {
402+
captureException(
403+
error instanceof Error ? error : new Error(String(error)),
404+
);
405+
} catch (captureExceptionError) {
406+
console.error(
407+
new Error(`Error thrown when calling 'captureException'`),
408+
captureExceptionError,
409+
);
410+
console.error(error);
411+
}
404412
} else {
405413
console.error(error);
406414
}

0 commit comments

Comments
 (0)