From 345fda74cb00dff066f4aaaf41c4722c4215b298 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= <kamil.ogorek@gmail.com>
Date: Mon, 21 Jun 2021 11:04:13 +0200
Subject: [PATCH] ref: Make beforeSend more strict (#3713)

---
 packages/core/src/baseclient.ts     | 38 ++++++++++++++++++++---------
 packages/core/test/lib/base.test.ts | 26 +++++++++++---------
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts
index 7ae94741b16c..f4e1825bff5b 100644
--- 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
index ffc0ee634c29..61e961d214d9 100644
--- 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 => {