ref: Make beforeSend more strict (#3713)

getsentry · Jun 21, 2021 · 345fda7 · 345fda7
1 parent 9cd2b7e
commit 345fda7
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 23 deletions.
diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts
@@ -13,6 +13,7 @@ import {
 import {
   dateTimestampInSeconds,
   Dsn,
+  isPlainObject,
   isPrimitive,
   isThenable,
   logger,
@@ -517,17 +518,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
         }
 
         const beforeSendResult = beforeSend(prepared, hint);
-        if (typeof beforeSendResult === 'undefined') {
-          throw new SentryError('`beforeSend` method has to return `null` or a valid event.');
-        } else if (isThenable(beforeSendResult)) {
-          return (beforeSendResult as PromiseLike<Event | null>).then(
-            event => event,
-            e => {
-              throw new SentryError(`beforeSend rejected with ${e}`);
-            },
-          );
-        }
-        return beforeSendResult;
+        return this._ensureBeforeSendRv(beforeSendResult);
       })
       .then(processedEvent => {
         if (processedEvent === null) {
@@ -575,4 +566,29 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
       },
     );
   }
+
+  /**
+   * Verifies that return value of configured `beforeSend` is of expected type.
+   */
+  protected _ensureBeforeSendRv(
+    rv: PromiseLike<Event | null> | Event | null,
+  ): PromiseLike<Event | null> | Event | null {
+    const nullErr = '`beforeSend` method has to return `null` or a valid event.';
+    if (isThenable(rv)) {
+      return (rv as PromiseLike<Event | null>).then(
+        event => {
+          if (!(isPlainObject(event) || event === null)) {
+            throw new SentryError(nullErr);
+          }
+          return event;
+        },
+        e => {
+          throw new SentryError(`beforeSend rejected with ${e}`);
+        },
+      );
+    } else if (!(isPlainObject(rv) || rv === null)) {
+      throw new SentryError(nullErr);
+    }
+    return rv;
+  }
 }
diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts
@@ -707,18 +707,20 @@ describe('BaseClient', () => {
     });
 
     test('calls beforeSend and log info about invalid return value', () => {
-      expect.assertions(3);
-      const beforeSend = jest.fn(() => undefined);
-      // @ts-ignore we need to test regular-js behavior
-      const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });
-      const captureExceptionSpy = jest.spyOn(client, 'captureException');
-      const loggerErrorSpy = jest.spyOn(logger, 'error');
-      client.captureEvent({ message: 'hello' });
-      expect(TestBackend.instance!.event).toBeUndefined();
-      expect(captureExceptionSpy).not.toBeCalled();
-      expect(loggerErrorSpy).toBeCalledWith(
-        new SentryError('`beforeSend` method has to return `null` or a valid event.'),
-      );
+      const invalidValues = [undefined, false, true, [], 1];
+      expect.assertions(invalidValues.length * 2);
+
+      for (const val of invalidValues) {
+        const beforeSend = jest.fn(() => val);
+        // @ts-ignore we need to test regular-js behavior
+        const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });
+        const loggerErrorSpy = jest.spyOn(logger, 'error');
+        client.captureEvent({ message: 'hello' });
+        expect(TestBackend.instance!.event).toBeUndefined();
+        expect(loggerErrorSpy).toBeCalledWith(
+          new SentryError('`beforeSend` method has to return `null` or a valid event.'),
+        );
+      }
     });
 
     test('calls async beforeSend and uses original event without any changes', done => {