Browse Source

bug(replays): Guard against hydrating invalid attachments (#51160)

Ryan Albrecht 1 year ago
parent
commit
11eb0b1629

+ 2 - 0
package.json

@@ -58,6 +58,7 @@
     "@types/crypto-js": "^4.1.1",
     "@types/diff": "5.0.2",
     "@types/dompurify": "^2.4.0",
+    "@types/invariant": "^2.2.35",
     "@types/jest": "^29.4.0",
     "@types/js-beautify": "^1.13.3",
     "@types/js-cookie": "^3.0.2",
@@ -108,6 +109,7 @@
     "gettext-parser": "1.3.1",
     "gl-matrix": "^3.4.3",
     "intersection-observer": "^0.12.2",
+    "invariant": "^2.2.4",
     "ios-device-list": "1.1.37",
     "jed": "^1.1.0",
     "js-beautify": "^1.14.4",

+ 13 - 0
static/app/utils/date/isValidDate.spec.tsx

@@ -0,0 +1,13 @@
+import isValidDate from 'sentry/utils/date/isValidDate';
+
+describe('isValidDate', () => {
+  it.each([
+    {label: 'date from string', expected: true, value: new Date('2023/01/01')},
+    {label: 'date from int', expected: true, value: new Date(123456)},
+    {label: 'date from NaN', expected: false, value: new Date(1 / 0)},
+    {label: 'duck type', expected: false, value: {getTime: () => 1}},
+    {label: 'object', expected: false, value: {foo: 'bar'}},
+  ])('should return {expected} for {label}', ({expected, value}) => {
+    expect(isValidDate(value)).toBe(expected);
+  });
+});

+ 8 - 0
static/app/utils/date/isValidDate.tsx

@@ -0,0 +1,8 @@
+/**
+ * Check that a value is a valid Date object, that doesn't point to NaN
+ *
+ * https://stackoverflow.com/questions/1353684/detecting-an-invalid-date-date-instance-in-javascript
+ */
+export default function isValidDate(d: unknown): d is Date {
+  return d instanceof Date && !isNaN(d.getTime());
+}

+ 21 - 13
static/app/utils/replays/hydrateBreadcrumbs.spec.tsx

@@ -6,8 +6,9 @@ import {BreadcrumbFrame} from 'sentry/utils/replays/types';
 const ONE_DAY_MS = 60 * 60 * 24 * 1000;
 
 describe('hydrateBreadcrumbs', () => {
+  const replayRecord = TestStubs.ReplayRecord({started_at: new Date('2023/12/23')});
+
   it('should set the timestampMs and offsetMs for each breadcrumb in the list', () => {
-    const replayRecord = TestStubs.ReplayRecord({started_at: new Date('2023/12/23')});
     const breadcrumbs = [
       TestStubs.Replay.ConsoleFrame({timestamp: new Date('2023/12/23')}),
       TestStubs.Replay.ConsoleFrame({timestamp: new Date('2023/12/24')}),
@@ -48,19 +49,26 @@ describe('hydrateBreadcrumbs', () => {
     ]);
   });
 
-  describe('replayInitBreadcrumb', () => {
-    it('should return a RecordingFrame', () => {
-      const replayRecord = TestStubs.ReplayRecord({});
+  it('should drop breadcrumbs that cannot be parsed', () => {
+    const breadcrumbs = [{foo: 'bar'}];
 
-      const frame: BreadcrumbFrame = replayInitBreadcrumb(replayRecord);
-      expect(frame).toStrictEqual({
-        category: 'replay.init',
-        message: 'http://localhost:3000/',
-        offsetMs: 0,
-        timestamp: replayRecord.started_at,
-        timestampMs: 1663865919000,
-        type: 'init',
-      });
+    // @ts-expect-error: Explicitly test invalid input
+    expect(hydrateBreadcrumbs(replayRecord, breadcrumbs)).toStrictEqual([]);
+  });
+});
+
+describe('replayInitBreadcrumb', () => {
+  it('should return a RecordingFrame', () => {
+    const replayRecord = TestStubs.ReplayRecord({});
+
+    const frame: BreadcrumbFrame = replayInitBreadcrumb(replayRecord);
+    expect(frame).toStrictEqual({
+      category: 'replay.init',
+      message: 'http://localhost:3000/',
+      offsetMs: 0,
+      timestamp: replayRecord.started_at,
+      timestampMs: 1663865919000,
+      type: 'init',
     });
   });
 });

+ 24 - 9
static/app/utils/replays/hydrateBreadcrumbs.tsx

@@ -1,22 +1,37 @@
+import invariant from 'invariant';
+
 import {BreadcrumbType} from 'sentry/types/breadcrumbs';
+import isValidDate from 'sentry/utils/date/isValidDate';
 import type {BreadcrumbFrame, RawBreadcrumbFrame} from 'sentry/utils/replays/types';
 import type {ReplayRecord} from 'sentry/views/replays/types';
 
+function isBreadcrumbFrame(frame: BreadcrumbFrame | undefined): frame is BreadcrumbFrame {
+  return frame !== undefined;
+}
+
 export default function hydrateBreadcrumbs(
   replayRecord: ReplayRecord,
   breadcrumbFrames: RawBreadcrumbFrame[]
 ): BreadcrumbFrame[] {
   const startTimestampMs = replayRecord.started_at.getTime();
 
-  return breadcrumbFrames.map((frame: RawBreadcrumbFrame) => {
-    const time = new Date(frame.timestamp * 1000);
-    return {
-      ...frame,
-      offsetMs: Math.abs(time.getTime() - startTimestampMs),
-      timestamp: time,
-      timestampMs: time.getTime(),
-    };
-  });
+  return breadcrumbFrames
+    .map((frame: RawBreadcrumbFrame) => {
+      try {
+        const time = new Date(frame.timestamp * 1000);
+        invariant(isValidDate(time), 'breadcrumbFrame.timestamp is invalid');
+
+        return {
+          ...frame,
+          offsetMs: Math.abs(time.getTime() - startTimestampMs),
+          timestamp: time,
+          timestampMs: time.getTime(),
+        };
+      } catch {
+        return undefined;
+      }
+    })
+    .filter(isBreadcrumbFrame);
 }
 
 export function replayInitBreadcrumb(replayRecord: ReplayRecord): BreadcrumbFrame {

+ 9 - 1
static/app/utils/replays/hydrateErrors.spec.tsx

@@ -3,8 +3,9 @@ import hydrateErrors from 'sentry/utils/replays/hydrateErrors';
 const ONE_DAY_MS = 60 * 60 * 24 * 1000;
 
 describe('hydrateErrors', () => {
+  const replayRecord = TestStubs.ReplayRecord({started_at: new Date('2023/12/23')});
+
   it('should set the timestamp & offsetMs for each span in the list', () => {
-    const replayRecord = TestStubs.ReplayRecord({started_at: new Date('2023/12/23')});
     const errors = [
       TestStubs.Replay.RawReplayError({timestamp: new Date('2023/12/23')}),
       TestStubs.Replay.RawReplayError({timestamp: new Date('2023/12/24')}),
@@ -59,4 +60,11 @@ describe('hydrateErrors', () => {
       },
     ]);
   });
+
+  it('should drop errors that cannot be parsed', () => {
+    const errors = [{foo: 'bar'}];
+
+    // @ts-expect-error: Explicitly test invalid input
+    expect(hydrateErrors(replayRecord, errors)).toStrictEqual([]);
+  });
 });

+ 36 - 22
static/app/utils/replays/hydrateErrors.tsx

@@ -1,32 +1,46 @@
-import {BreadcrumbType} from 'sentry/types/breadcrumbs';
+import invariant from 'invariant';
+
+import isValidDate from 'sentry/utils/date/isValidDate';
 import type {ErrorFrame, RawReplayError} from 'sentry/utils/replays/types';
 import type {ReplayRecord} from 'sentry/views/replays/types';
 
+function isErrorFrame(frame: ErrorFrame | undefined): frame is ErrorFrame {
+  return frame !== undefined;
+}
+
 export default function hydrateErrors(
   replayRecord: ReplayRecord,
   errors: RawReplayError[]
 ): ErrorFrame[] {
   const startTimestampMs = replayRecord.started_at.getTime();
 
-  return errors.map(error => {
-    const time = new Date(error.timestamp);
-    return {
-      category: 'issue',
-      data: {
-        eventId: error.id,
-        groupId: error['issue.id'],
-        groupShortId: error.issue,
-        label:
-          (Array.isArray(error['error.type'])
-            ? error['error.type'][0]
-            : error['error.type']) ?? '',
-        projectSlug: error['project.name'],
-      },
-      message: error.title,
-      offsetMs: Math.abs(time.getTime() - startTimestampMs),
-      timestamp: time,
-      timestampMs: time.getTime(),
-      type: BreadcrumbType.ERROR, // For compatibility reasons. See BreadcrumbType
-    };
-  });
+  return errors
+    .map(error => {
+      try {
+        const time = new Date(error.timestamp);
+        invariant(isValidDate(time), 'errorFrame.timestamp is invalid');
+
+        return {
+          category: 'issue' as const,
+          data: {
+            eventId: error.id,
+            groupId: error['issue.id'],
+            groupShortId: error.issue,
+            label:
+              (Array.isArray(error['error.type'])
+                ? error['error.type'][0]
+                : error['error.type']) ?? '',
+            projectSlug: error['project.name'],
+          },
+          message: error.title,
+          offsetMs: Math.abs(time.getTime() - startTimestampMs),
+          timestamp: time,
+          timestampMs: time.getTime(),
+          type: 'error', // For compatibility reasons. See BreadcrumbType
+        };
+      } catch {
+        return undefined;
+      }
+    })
+    .filter(isErrorFrame);
 }

+ 9 - 1
static/app/utils/replays/hydrateSpans.spec.tsx

@@ -3,8 +3,9 @@ import hydrateSpans from 'sentry/utils/replays/hydrateSpans';
 const ONE_DAY_MS = 60 * 60 * 24 * 1000;
 
 describe('hydrateSpans', () => {
+  const replayRecord = TestStubs.ReplayRecord({started_at: new Date('2023/12/23')});
+
   it('should set the start & end timestamps, & offsetMs for each span in the list', () => {
-    const replayRecord = TestStubs.ReplayRecord({started_at: new Date('2023/12/23')});
     const spans = [
       TestStubs.Replay.MemoryFrame({
         startTimestamp: new Date('2023/12/23'),
@@ -50,4 +51,11 @@ describe('hydrateSpans', () => {
       },
     ]);
   });
+
+  it('should drop spans that cannot be parsed', () => {
+    const spans = [{foo: 'bar'}];
+
+    // @ts-expect-error: Explicitly test invalid input
+    expect(hydrateSpans(replayRecord, spans)).toStrictEqual([]);
+  });
 });

+ 29 - 13
static/app/utils/replays/hydrateSpans.tsx

@@ -1,24 +1,40 @@
+import invariant from 'invariant';
+
+import isValidDate from 'sentry/utils/date/isValidDate';
 import type {RawSpanFrame, SpanFrame} from 'sentry/utils/replays/types';
 import type {ReplayRecord} from 'sentry/views/replays/types';
 
+function isSpanFrame(frame: SpanFrame | undefined): frame is SpanFrame {
+  return frame !== undefined;
+}
+
 export default function hydrateSpans(
   replayRecord: ReplayRecord,
   spanFrames: RawSpanFrame[]
 ): SpanFrame[] {
   const startTimestampMs = replayRecord.started_at.getTime();
 
-  return spanFrames.map((frame: RawSpanFrame) => {
-    const start = new Date(frame.startTimestamp * 1000);
-    const end = new Date(frame.endTimestamp * 1000);
-    return {
-      ...frame,
-      endTimestamp: end,
-      offsetMs: Math.abs(start.getTime() - startTimestampMs),
-      startTimestamp: start,
-      timestampMs: start.getTime(),
+  return spanFrames
+    .map((frame: RawSpanFrame) => {
+      try {
+        const start = new Date(frame.startTimestamp * 1000);
+        const end = new Date(frame.endTimestamp * 1000);
+
+        invariant(isValidDate(start), 'spanFrame.startTimestamp is invalid');
+        invariant(isValidDate(end), 'spanFrame.endTimestamp is invalid');
+        return {
+          ...frame,
+          endTimestamp: end,
+          offsetMs: Math.abs(start.getTime() - startTimestampMs),
+          startTimestamp: start,
+          timestampMs: start.getTime(),
 
-      // TODO: do we need this added as well?
-      // id: `${span.description ?? span.op}-${span.startTimestamp}-${span.endTimestamp}`,
-    };
-  });
+          // TODO: do we need this added as well?
+          // id: `${span.description ?? span.op}-${span.startTimestamp}-${span.endTimestamp}`,
+        };
+      } catch {
+        return undefined;
+      }
+    })
+    .filter(isSpanFrame);
 }

+ 2 - 0
static/app/utils/replays/types.tsx

@@ -172,6 +172,7 @@ export type RawReplayError = {
 export type ErrorFrame = Overwrite<
   BreadcrumbFrame,
   {
+    category: 'issue';
     data: {
       eventId: string; // error['id']
       groupId: number; // error['issue.id']
@@ -179,5 +180,6 @@ export type ErrorFrame = Overwrite<
       label: string; // error['error.type'].join('')
       projectSlug: string; // error['project.name']
     };
+    message: string;
   }
 >;

Some files were not shown because too many files changed in this diff