fix: Cannot create proxy with a non-object as target or handler

Some users are storing original JWT (as string) inside authInfo/securityContext by using custom checkAuth or checkAuthMiddleware, which is unexpected (must be object) and can cause this issue. I started to use {} as the default value for securityContext when it's not an object before passing it to schema-compiler. I've added a warning when a user is storing not object inside securityContext to indicate that they are doing something wrong.

packages/cubejs-api-gateway/package.json

@@ -27,6 +27,7 @@
     "dist/src/*"
   ],
   "dependencies": {
+    "@cubejs-backend/shared": "^0.26.0",
     "@hapi/joi": "^15.1.1",
     "body-parser": "^1.19.0",
     "chrono-node": "1.4.4",
@@ -39,7 +40,6 @@
   },
   "devDependencies": {
     "@cubejs-backend/linter": "^0.26.0",
-    "@cubejs-backend/shared": "^0.26.0",
     "@types/express": "^4.17.9",
     "@types/hapi__joi": "^15.0.4",
     "@types/jest": "^26.0.20",

packages/cubejs-api-gateway/src/gateway.ts

@@ -2,6 +2,7 @@ import jwt from 'jsonwebtoken';
 import R from 'ramda';
 import moment from 'moment';
 import bodyParser from 'body-parser';
+import { getRealType } from '@cubejs-backend/shared';
 
 import type {
   Response, NextFunction,
@@ -404,7 +405,7 @@ export class ApiGateway {
   protected coerceForSqlQuery(query, context: RequestContext) {
     let securityContext = {};
 
-    if (context.securityContext) {
+    if (typeof context.securityContext === 'object' && context.securityContext !== null) {
       if (context.securityContext.u) {
         if (!this.checkAuthDeprecationShown) {
           this.logger('JWT U Property Deprecation', {
@@ -731,6 +732,9 @@ export class ApiGateway {
       )
     });
 
+    // securityContext should be object
+    let showWarningAboutNotObject = false;
+
     return (req, res, next) => {
       fn(req, res, (e) => {
         // We renamed authInfo to securityContext, but users can continue to use both ways
@@ -740,6 +744,16 @@ export class ApiGateway {
           req.securityContext = req.authInfo;
         }
 
+        if ((typeof req.securityContext !== 'object' || req.securityContext === null) && !showWarningAboutNotObject) {
+          this.logger('Security Context Should Be Object', {
+            warning: (
+              `Value of securityContext (previously authInfo) expected to be object, actual: ${getRealType(req.securityContext)}`
+            )
+          });
+
+          showWarningAboutNotObject = true;
+        }
+
         next(e);
       });
     };
@@ -748,6 +762,8 @@ export class ApiGateway {
   protected wrapCheckAuth(fn: CheckAuthFn): CheckAuthFn {
     // We dont need to span all logs with deprecation message
     let warningShowed = false;
+    // securityContext should be object
+    let showWarningAboutNotObject = false;
 
     return async (req, auth) => {
       await fn(req, auth);
@@ -769,6 +785,16 @@ export class ApiGateway {
 
         req.securityContext = req.authInfo;
       }
+
+      if ((typeof req.securityContext !== 'object' || req.securityContext === null) && !showWarningAboutNotObject) {
+        this.logger('Security Context Should Be Object', {
+          warning: (
+            `Value of securityContext (previously authInfo) expected to be object, actual: ${getRealType(req.securityContext)}`
+          )
+        });
+
+        showWarningAboutNotObject = true;
+      }
     };
   }
 

packages/cubejs-api-gateway/test/auth.test.ts

@@ -105,7 +105,7 @@ describe('test authorization', () => {
       res.status(200).end();
     });
 
-    const { apiGateway, app } = createApiGateway(handlerMock, loggerMock, {});
+    const { app } = createApiGateway(handlerMock, loggerMock, {});
 
     await request(app)
       .get('/test-auth-fake')
@@ -115,14 +115,6 @@ describe('test authorization', () => {
 
     expect(loggerMock.mock.calls.length).toEqual(0);
     expect(handlerMock.mock.calls.length).toEqual(1);
-
-    const args: any = handlerMock.mock.calls[0];
-
-    expect(apiGateway.coerceForSqlQuery({ timeDimensions: [] }, args[0]).contextSymbols.securityContext).toEqual({
-      exp: 2475858836,
-      iat: 1611858836,
-      uid: 5,
-    });
   });
 
   test('custom checkAuth with async flow', async () => {
@@ -217,6 +209,50 @@ describe('test authorization', () => {
     expect(handlerMock.mock.calls[0][0].context.authInfo).toEqual(EXPECTED_SECURITY_CONTEXT);
   });
 
+  test('custom checkAuth with securityContext (not object)', async () => {
+    const loggerMock = jest.fn(() => {
+      //
+    });
+
+    const EXPECTED_SECURITY_CONTEXT = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1aWQiOjUsImlhdCI6MTYxMTg1NzcwNSwiZXhwIjoyNDc1ODU3NzA1fQ.tTieqdIcxDLG8fHv8YWwfvg_rPVe1XpZKUvrCdzVn3g';
+
+    const handlerMock = jest.fn((req, res) => {
+      expect(req.context.securityContext).toEqual(EXPECTED_SECURITY_CONTEXT);
+      expect(req.context.authInfo).toEqual(EXPECTED_SECURITY_CONTEXT);
+
+      res.status(200).end();
+    });
+
+    const { app } = createApiGateway(handlerMock, loggerMock, {
+      checkAuth: (req: Request, auth?: string) => {
+        if (auth) {
+          // It must be object, but some users are using string for securityContext
+          req.securityContext = auth;
+        }
+      }
+    });
+
+    await request(app)
+      .get('/test-auth-fake')
+      // console.log(generateAuthToken({ uid: 5, }));
+      .set('Authorization', 'Authorization: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1aWQiOjUsImlhdCI6MTYxMTg1NzcwNSwiZXhwIjoyNDc1ODU3NzA1fQ.tTieqdIcxDLG8fHv8YWwfvg_rPVe1XpZKUvrCdzVn3g')
+      .expect(200);
+
+    expect(loggerMock.mock.calls.length).toEqual(1);
+    expect(loggerMock.mock.calls[0]).toEqual([
+      'Security Context Should Be Object',
+      {
+        warning: 'Value of securityContext (previously authInfo) expected to be object, actual: string',
+      }
+    ]);
+
+    expect(handlerMock.mock.calls.length).toEqual(1);
+
+    expect(handlerMock.mock.calls[0][0].context.securityContext).toEqual(EXPECTED_SECURITY_CONTEXT);
+    // authInfo was deprecated, but should exists as computability
+    expect(handlerMock.mock.calls[0][0].context.authInfo).toEqual(EXPECTED_SECURITY_CONTEXT);
+  });
+
   test('custom checkAuthMiddleware with deprecated authInfo', async () => {
     const loggerMock = jest.fn(() => {
       //
@@ -258,11 +294,113 @@ describe('test authorization', () => {
         warning: 'Option checkAuthMiddleware is now deprecated in favor of checkAuth, please migrate: https://github.com/cube-js/cube.js/blob/master/DEPRECATION.md#checkauthmiddleware',
       }
     ]);
-
     expect(handlerMock.mock.calls.length).toEqual(1);
 
     expect(handlerMock.mock.calls[0][0].context.securityContext).toEqual(EXPECTED_SECURITY_CONTEXT);
     // authInfo was deprecated, but should exists as computability
     expect(handlerMock.mock.calls[0][0].context.authInfo).toEqual(EXPECTED_SECURITY_CONTEXT);
   });
+
+  test('custom checkAuthMiddleware with securityInfo (not object)', async () => {
+    const loggerMock = jest.fn(() => {
+      //
+    });
+
+    const EXPECTED_SECURITY_CONTEXT = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1aWQiOjUsImlhdCI6MTYxMTg1NzcwNSwiZXhwIjoyNDc1ODU3NzA1fQ.tTieqdIcxDLG8fHv8YWwfvg_rPVe1XpZKUvrCdzVn3g';
+
+    const handlerMock = jest.fn((req, res) => {
+      expect(req.context.securityContext).toEqual(EXPECTED_SECURITY_CONTEXT);
+      expect(req.context.authInfo).toEqual(EXPECTED_SECURITY_CONTEXT);
+
+      res.status(200).end();
+    });
+
+    const { app } = createApiGateway(handlerMock, loggerMock, {
+      checkAuthMiddleware: (req: Request, res, next) => {
+        if (req.headers.authorization) {
+          // It must be object, but some users are using string for securityContext
+          req.authInfo = req.headers.authorization;
+        }
+
+        if (next) {
+          next();
+        }
+      }
+    });
+
+    await request(app)
+      .get('/test-auth-fake')
+      // console.log(generateAuthToken({ uid: 5, }));
+      .set('Authorization', 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1aWQiOjUsImlhdCI6MTYxMTg1NzcwNSwiZXhwIjoyNDc1ODU3NzA1fQ.tTieqdIcxDLG8fHv8YWwfvg_rPVe1XpZKUvrCdzVn3g')
+      .expect(200);
+
+    expect(loggerMock.mock.calls.length).toEqual(2);
+    expect(loggerMock.mock.calls[0]).toEqual([
+      'CheckAuthMiddleware Middleware Deprecation',
+      {
+        warning: 'Option checkAuthMiddleware is now deprecated in favor of checkAuth, please migrate: https://github.com/cube-js/cube.js/blob/master/DEPRECATION.md#checkauthmiddleware',
+      }
+    ]);
+    expect(loggerMock.mock.calls[1]).toEqual([
+      'Security Context Should Be Object',
+      {
+        warning: 'Value of securityContext (previously authInfo) expected to be object, actual: string',
+      }
+    ]);
+
+    expect(handlerMock.mock.calls.length).toEqual(1);
+    expect(handlerMock.mock.calls[0][0].context.securityContext).toEqual(EXPECTED_SECURITY_CONTEXT);
+    // authInfo was deprecated, but should exists as computability
+    expect(handlerMock.mock.calls[0][0].context.authInfo).toEqual(EXPECTED_SECURITY_CONTEXT);
+  });
+
+  test('coerceForSqlQuery multiple', async () => {
+    const loggerMock = jest.fn(() => {
+      //
+    });
+
+    const handlerMock = jest.fn();
+
+    const { apiGateway } = createApiGateway(handlerMock, loggerMock, {});
+
+    // handle null
+    expect(
+      apiGateway.coerceForSqlQuery(
+        { timeDimensions: [] },
+        { securityContext: null, requestId: 'XXX' }
+      ).contextSymbols.securityContext
+    ).toEqual({});
+    // no warnings, done on checkAuth/checkAuthMiddleware level
+    expect(loggerMock.mock.calls.length).toEqual(0);
+
+    // handle string
+    expect(
+      apiGateway.coerceForSqlQuery(
+        { timeDimensions: [] },
+        { securityContext: 'AAABBBCCC', requestId: 'XXX' }
+      ).contextSymbols.securityContext
+    ).toEqual({});
+    // no warnings, done on checkAuth/checkAuthMiddleware level
+    expect(loggerMock.mock.calls.length).toEqual(0);
+
+    // (move u to root)
+    expect(
+      apiGateway.coerceForSqlQuery(
+        { timeDimensions: [] },
+        { securityContext: { exp: 2475858836, iat: 1611858836, u: { uid: 5 } }, requestId: 'XXX' }
+      ).contextSymbols.securityContext
+    ).toEqual({
+      exp: 2475858836,
+      iat: 1611858836,
+      uid: 5,
+    });
+
+    expect(loggerMock.mock.calls.length).toEqual(1);
+    expect(loggerMock.mock.calls[0]).toEqual([
+      'JWT U Property Deprecation',
+      {
+        warning: 'Storing security context in the u property within the payload is now deprecated, please migrate: https://github.com/cube-js/cube.js/blob/master/DEPRECATION.md#authinfo',
+      }
+    ]);
+  });
 });

packages/cubejs-backend-shared/src/helpers.ts

@@ -0,0 +1,7 @@
+export function getRealType(value: any): string {
+  if (value === null) {
+    return 'null';
+  }
+
+  return typeof value;
+}

packages/cubejs-backend-shared/src/index.ts

@@ -4,3 +4,4 @@ export * from './track';
 export * from './errors';
 export * from './promises';
 export * from './convert';
+export * from './helpers';

packages/cubejs-backend-shared/test/helper.test.ts

@@ -0,0 +1,7 @@
+import { getRealType } from '../src';
+
+test('getRealType', () => {
+  expect(getRealType(1)).toBe('number');
+  expect(getRealType({})).toBe('object');
+  expect(getRealType(null)).toBe('null');
+});