fix: Guard against invalid req.user input (getsentry#2512)

kamilogorek · web-flow · commit 67934520fb23 · 2020-03-25T14:16:51.000+01:00
diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts
@@ -1,7 +1,7 @@
 import { Span } from '@sentry/apm';
 import { captureException, getCurrentHub, withScope } from '@sentry/core';
 import { Event } from '@sentry/types';
-import { forget, isString, logger, normalize } from '@sentry/utils';
+import { forget, isPlainObject, isString, logger, normalize } from '@sentry/utils';
 import * as cookie from 'cookie';
 import * as domain from 'domain';
 import * as http from 'http';
@@ -256,7 +256,7 @@ export function parseRequest(
   }
 
   if (options.user) {
-    const extractedUser = req.user ? extractUserData(req.user, options.user) : {};
+    const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, options.user) : {};
 
     if (Object.keys(extractedUser)) {
       event.user = {
diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts
@@ -1,4 +1,6 @@
-import { Event } from '../src';
+import { Runtime } from '@sentry/types';
+
+import { Event, Request, User } from '../src';
 import { parseRequest } from '../src/handlers';
 
 describe('parseRequest', () => {
@@ -21,12 +23,12 @@ describe('parseRequest', () => {
   describe('parseRequest.contexts runtime', () => {
     test('runtime name must contain node', () => {
       const parsedRequest: Event = parseRequest({}, mockReq);
-      expect(parsedRequest.contexts.runtime.name).toEqual('node');
+      expect((parsedRequest.contexts!.runtime as Runtime).name).toEqual('node');
     });
 
     test('runtime version must contain current node version', () => {
       const parsedRequest: Event = parseRequest({}, mockReq);
-      expect(parsedRequest.contexts.runtime.version).toEqual(process.version);
+      expect((parsedRequest.contexts!.runtime as Runtime).version).toEqual(process.version);
     });
 
     test('runtime disbaled by options', () => {
@@ -42,17 +44,27 @@ describe('parseRequest', () => {
     const CUSTOM_USER_KEYS = ['custom_property'];
 
     test('parseRequest.user only contains the default properties from the user', () => {
-      const parsedRequest: Event = parseRequest({}, mockReq, {
-        user: DEFAULT_USER_KEYS,
-      });
-      expect(Object.keys(parsedRequest.user as any[])).toEqual(DEFAULT_USER_KEYS);
+      const parsedRequest: Event = parseRequest({}, mockReq);
+      expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS);
     });
 
     test('parseRequest.user only contains the custom properties specified in the options.user array', () => {
       const parsedRequest: Event = parseRequest({}, mockReq, {
         user: CUSTOM_USER_KEYS,
       });
-      expect(Object.keys(parsedRequest.user as any[])).toEqual(CUSTOM_USER_KEYS);
+      expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS);
+    });
+
+    test('parseRequest.user doesnt blow up when someone passes non-object value', () => {
+      const parsedRequest: Event = parseRequest(
+        {},
+        {
+          ...mockReq,
+          // @ts-ignore
+          user: 'wat',
+        },
+      );
+      expect(Object.keys(parsedRequest.user as User)).toEqual([]);
     });
   });
 
@@ -92,15 +104,15 @@ describe('parseRequest', () => {
     test('parseRequest.request only contains the default set of properties from the request', () => {
       const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
       const parsedRequest: Event = parseRequest({}, mockReq);
-      expect(Object.keys(parsedRequest.request as {})).toEqual(DEFAULT_REQUEST_PROPERTIES);
+      expect(Object.keys(parsedRequest.request as Request)).toEqual(DEFAULT_REQUEST_PROPERTIES);
     });
 
     test('parseRequest.request only contains the specified properties in the options.request array', () => {
       const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url'];
       const parsedRequest: Event = parseRequest({}, mockReq, {
         request: INCLUDED_PROPERTIES,
       });
-      expect(Object.keys(parsedRequest.request as {})).toEqual(INCLUDED_PROPERTIES);
+      expect(Object.keys(parsedRequest.request as Request)).toEqual(INCLUDED_PROPERTIES);
     });
 
     test('parseRequest.request skips `body` property for GET and HEAD requests', () => {
diff --git a/packages/types/src/event.ts b/packages/types/src/event.ts
@@ -30,7 +30,9 @@ export interface Event {
   };
   stacktrace?: Stacktrace;
   breadcrumbs?: Breadcrumb[];
-  contexts?: { [key: string]: object };
+  contexts?: {
+    [key: string]: object;
+  };
   tags?: { [key: string]: string };
   extra?: { [key: string]: any };
   user?: User;
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
@@ -13,6 +13,7 @@ export { Options } from './options';
 export { Package } from './package';
 export { Request } from './request';
 export { Response } from './response';
+export { Runtime } from './runtime';
 export { Scope } from './scope';
 export { SdkInfo } from './sdkinfo';
 export { Severity } from './severity';
