From 0d293929f548c9869a1b7c0016702980c37e30a7 Mon Sep 17 00:00:00 2001 From: olav Date: Fri, 19 Aug 2022 08:09:44 +0200 Subject: [PATCH] feat: add CORS support to the proxy endpoints (#1936) * feat: add CORS support to the proxy endpoints * refactor: remove unused development mode CORS support --- package.json | 1 + .../__snapshots__/create-config.test.ts.snap | 1 + src/lib/app.ts | 19 ++++++++--- src/lib/create-config.ts | 11 ++++++- .../middleware/cors-origin-middleware.test.ts | 18 +++++++++++ src/lib/middleware/cors-origin-middleware.ts | 25 +++++++++++++++ src/lib/routes/proxy-api/index.ts | 8 ++++- src/lib/types/option.ts | 2 ++ .../util/{env.test.ts => parseEnvVar.test.ts} | 16 +++++++++- src/lib/util/{env.ts => parseEnvVar.ts} | 14 ++++++++ src/test/e2e/api/proxy/proxy.e2e.test.ts | 32 +++++++++++++++++-- yarn.lock | 5 +++ 12 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 src/lib/middleware/cors-origin-middleware.test.ts create mode 100644 src/lib/middleware/cors-origin-middleware.ts rename src/lib/util/{env.test.ts => parseEnvVar.test.ts} (64%) rename src/lib/util/{env.ts => parseEnvVar.ts} (62%) diff --git a/package.json b/package.json index 21e19940ab..b1366ecd8c 100644 --- a/package.json +++ b/package.json @@ -132,6 +132,7 @@ "@apidevtools/swagger-parser": "10.1.0", "@babel/core": "7.18.10", "@types/bcryptjs": "2.4.2", + "@types/cors": "^2.8.12", "@types/express": "4.17.13", "@types/express-session": "1.17.5", "@types/faker": "5.5.9", diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 4a1f07ce88..0524f4409a 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -62,6 +62,7 @@ Object { }, "eventHook": undefined, "experimental": Object {}, + "frontendApiOrigins": Array [], "getLogger": [Function], "import": Object { "dropBeforeImport": false, diff --git a/src/lib/app.ts b/src/lib/app.ts index 1b6585d1cb..645d83fc09 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -1,12 +1,12 @@ import { publicFolder } from 'unleash-frontend'; import express, { Application, RequestHandler } from 'express'; -import cors from 'cors'; import compression from 'compression'; import favicon from 'serve-favicon'; import cookieParser from 'cookie-parser'; import path from 'path'; import errorHandler from 'errorhandler'; import { responseTimeMetrics } from './middleware/response-time-metrics'; +import { corsOriginMiddleware } from './middleware/cors-origin-middleware'; import rbacMiddleware from './middleware/rbac-middleware'; import apiTokenMiddleware from './middleware/api-token-middleware'; import { IUnleashServices } from './types/services'; @@ -49,10 +49,6 @@ export default async function getApp( config.preHook(app, config, services); } - if (process.env.NODE_ENV === 'development') { - app.use(cors()); - } - app.use(compression()); app.use(cookieParser()); app.use(express.json({ strict: false })); @@ -73,6 +69,19 @@ export default async function getApp( services.openApiService.useDocs(app); } + if ( + config.experimental.embedProxy && + config.frontendApiOrigins.length > 0 + ) { + // Support CORS preflight requests for the frontend endpoints. + // Preflight requests should not have Authorization headers, + // so this must be handled before the API token middleware. + app.options( + '/api/frontend*', + corsOriginMiddleware(config.frontendApiOrigins), + ); + } + switch (config.authentication.type) { case IAuthType.OPEN_SOURCE: { app.use(baseUriPath, apiTokenMiddleware(config, services)); diff --git a/src/lib/create-config.ts b/src/lib/create-config.ts index 934ca303b5..7b2fb8de2c 100644 --- a/src/lib/create-config.ts +++ b/src/lib/create-config.ts @@ -29,7 +29,11 @@ import { mapLegacyToken, validateApiToken, } from './types/models/api-token'; -import { parseEnvVarBoolean, parseEnvVarNumber } from './util/env'; +import { + parseEnvVarBoolean, + parseEnvVarNumber, + parseEnvVarStrings, +} from './util/parseEnvVar'; import { IExperimentalOptions } from './experimental'; import { DEFAULT_SEGMENT_VALUES_LIMIT, @@ -402,6 +406,10 @@ export function createConfig(options: IUnleashOptions): IUnleashConfig { DEFAULT_STRATEGY_SEGMENTS_LIMIT, ); + const frontendApiOrigins = + options.frontendApiOrigins || + parseEnvVarStrings(process.env.UNLEASH_FRONTEND_API_ORIGINS, []); + const clientFeatureCaching = loadClientCachingOptions(options); return { @@ -426,6 +434,7 @@ export function createConfig(options: IUnleashOptions): IUnleashConfig { eventBus: new EventEmitter(), environmentEnableOverrides, additionalCspAllowedDomains, + frontendApiOrigins, inlineSegmentConstraints, segmentValuesLimit, strategySegmentsLimit, diff --git a/src/lib/middleware/cors-origin-middleware.test.ts b/src/lib/middleware/cors-origin-middleware.test.ts new file mode 100644 index 0000000000..82ffc43df4 --- /dev/null +++ b/src/lib/middleware/cors-origin-middleware.test.ts @@ -0,0 +1,18 @@ +import { allowRequestOrigin } from './cors-origin-middleware'; + +test('allowRequestOrigin', () => { + const dotCom = 'https://example.com'; + const dotOrg = 'https://example.org'; + + expect(allowRequestOrigin('', [])).toEqual(false); + expect(allowRequestOrigin(dotCom, [])).toEqual(false); + expect(allowRequestOrigin(dotCom, [dotOrg])).toEqual(false); + + expect(allowRequestOrigin(dotCom, [dotCom, dotOrg])).toEqual(true); + expect(allowRequestOrigin(dotCom, [dotOrg, dotCom])).toEqual(true); + expect(allowRequestOrigin(dotCom, [dotCom, dotCom])).toEqual(true); + + expect(allowRequestOrigin(dotCom, ['*'])).toEqual(true); + expect(allowRequestOrigin(dotCom, [dotOrg, '*'])).toEqual(true); + expect(allowRequestOrigin(dotCom, [dotCom, dotOrg, '*'])).toEqual(true); +}); diff --git a/src/lib/middleware/cors-origin-middleware.ts b/src/lib/middleware/cors-origin-middleware.ts new file mode 100644 index 0000000000..5e04abb98f --- /dev/null +++ b/src/lib/middleware/cors-origin-middleware.ts @@ -0,0 +1,25 @@ +import { RequestHandler } from 'express'; +import cors from 'cors'; + +const ANY_ORIGIN = '*'; + +export const allowRequestOrigin = ( + requestOrigin: string, + allowedOrigins: string[], +): boolean => { + return allowedOrigins.some((allowedOrigin) => { + return allowedOrigin === requestOrigin || allowedOrigin === ANY_ORIGIN; + }); +}; + +// Check the request's Origin header against a list of allowed origins. +// The list may include '*', which `cors` does not support natively. +export const corsOriginMiddleware = ( + allowedOrigins: string[], +): RequestHandler => { + return cors((req, callback) => { + callback(null, { + origin: allowRequestOrigin(req.header('Origin'), allowedOrigins), + }); + }); +}; diff --git a/src/lib/routes/proxy-api/index.ts b/src/lib/routes/proxy-api/index.ts index de46c7cea1..d0fc73429b 100644 --- a/src/lib/routes/proxy-api/index.ts +++ b/src/lib/routes/proxy-api/index.ts @@ -17,6 +17,7 @@ import { ProxyClientSchema } from '../../openapi/spec/proxy-client-schema'; import { createResponseSchema } from '../../openapi/util/create-response-schema'; import { createRequestSchema } from '../../openapi/util/create-request-schema'; import { emptyResponse } from '../../openapi/util/standard-responses'; +import { corsOriginMiddleware } from '../../middleware/cors-origin-middleware'; interface ApiUserRequest< PARAM = any, @@ -34,7 +35,6 @@ export default class ProxyController extends Controller { private openApiService: OpenApiService; - // TODO(olav): Add CORS config to all proxy endpoints. constructor( config: IUnleashConfig, { @@ -47,6 +47,12 @@ export default class ProxyController extends Controller { this.proxyService = proxyService; this.openApiService = openApiService; + if (config.frontendApiOrigins.length > 0) { + // Support CORS requests for the frontend endpoints. + // Preflight requests are handled in `app.ts`. + this.app.use(corsOriginMiddleware(config.frontendApiOrigins)); + } + this.route({ method: 'get', path: '', diff --git a/src/lib/types/option.ts b/src/lib/types/option.ts index 50b94e8704..f3cd0cbfff 100644 --- a/src/lib/types/option.ts +++ b/src/lib/types/option.ts @@ -106,6 +106,7 @@ export interface IUnleashOptions { email?: Partial; secureHeaders?: boolean; additionalCspAllowedDomains?: ICspDomainOptions; + frontendApiOrigins?: string[]; enableOAS?: boolean; preHook?: Function; preRouterHook?: Function; @@ -179,6 +180,7 @@ export interface IUnleashConfig { email: IEmailOption; secureHeaders: boolean; additionalCspAllowedDomains: ICspDomainConfig; + frontendApiOrigins: string[]; enableOAS: boolean; preHook?: Function; preRouterHook?: Function; diff --git a/src/lib/util/env.test.ts b/src/lib/util/parseEnvVar.test.ts similarity index 64% rename from src/lib/util/env.test.ts rename to src/lib/util/parseEnvVar.test.ts index 13cc809ad1..a147e6cca9 100644 --- a/src/lib/util/env.test.ts +++ b/src/lib/util/parseEnvVar.test.ts @@ -1,4 +1,8 @@ -import { parseEnvVarBoolean, parseEnvVarNumber } from './env'; +import { + parseEnvVarBoolean, + parseEnvVarNumber, + parseEnvVarStrings, +} from './parseEnvVar'; test('parseEnvVarNumber', () => { expect(parseEnvVarNumber('', 1)).toEqual(1); @@ -23,3 +27,13 @@ test('parseEnvVarBoolean', () => { expect(parseEnvVarBoolean('false', false)).toEqual(false); expect(parseEnvVarBoolean('test', false)).toEqual(false); }); + +test('parseEnvVarStringList', () => { + expect(parseEnvVarStrings('', [])).toEqual([]); + expect(parseEnvVarStrings(' ', [])).toEqual([]); + expect(parseEnvVarStrings('', ['*'])).toEqual(['*']); + expect(parseEnvVarStrings('a', ['*'])).toEqual(['a']); + expect(parseEnvVarStrings('a,b,c', [])).toEqual(['a', 'b', 'c']); + expect(parseEnvVarStrings('a,b,c', [])).toEqual(['a', 'b', 'c']); + expect(parseEnvVarStrings(' a,,,b, c , ,', [])).toEqual(['a', 'b', 'c']); +}); diff --git a/src/lib/util/env.ts b/src/lib/util/parseEnvVar.ts similarity index 62% rename from src/lib/util/env.ts rename to src/lib/util/parseEnvVar.ts index b8b603ae66..a52ada078e 100644 --- a/src/lib/util/env.ts +++ b/src/lib/util/parseEnvVar.ts @@ -18,3 +18,17 @@ export function parseEnvVarBoolean( return defaultVal; } + +export function parseEnvVarStrings( + envVar: string, + defaultVal: string[], +): string[] { + if (envVar) { + return envVar + .split(',') + .map((item) => item.trim()) + .filter(Boolean); + } + + return defaultVal; +} diff --git a/src/test/e2e/api/proxy/proxy.e2e.test.ts b/src/test/e2e/api/proxy/proxy.e2e.test.ts index 8e2252fe26..cf1f053cb4 100644 --- a/src/test/e2e/api/proxy/proxy.e2e.test.ts +++ b/src/test/e2e/api/proxy/proxy.e2e.test.ts @@ -15,7 +15,9 @@ let db: ITestDb; beforeAll(async () => { db = await dbInit('proxy', getLogger); - app = await setupAppWithAuth(db.stores); + app = await setupAppWithAuth(db.stores, { + frontendApiOrigins: ['https://example.com'], + }); }); afterAll(async () => { @@ -179,8 +181,32 @@ test('should return 405 from unimplemented endpoints', async () => { .expect(405); }); -// TODO(olav): Test CORS config for all proxy endpoints. -test.todo('should enforce token CORS settings'); +test('should enforce frontend API CORS config', async () => { + const allowedOrigin = 'https://example.com'; + const unknownOrigin = 'https://example.org'; + const origin = 'access-control-allow-origin'; + const frontendToken = await createApiToken(ApiTokenType.FRONTEND); + await app.request + .options('/api/frontend') + .set('Origin', unknownOrigin) + .set('Authorization', frontendToken.secret) + .expect((res) => expect(res.headers[origin]).toBeUndefined()); + await app.request + .options('/api/frontend') + .set('Origin', allowedOrigin) + .set('Authorization', frontendToken.secret) + .expect((res) => expect(res.headers[origin]).toEqual(allowedOrigin)); + await app.request + .get('/api/frontend') + .set('Origin', unknownOrigin) + .set('Authorization', frontendToken.secret) + .expect((res) => expect(res.headers[origin]).toBeUndefined()); + await app.request + .get('/api/frontend') + .set('Origin', allowedOrigin) + .set('Authorization', frontendToken.secret) + .expect((res) => expect(res.headers[origin]).toEqual(allowedOrigin)); +}); test('should accept client registration requests', async () => { const frontendToken = await createApiToken(ApiTokenType.FRONTEND); diff --git a/yarn.lock b/yarn.lock index f8b81c4311..99cff1c7bc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1103,6 +1103,11 @@ resolved "https://registry.npmjs.org/@types/cookiejar/-/cookiejar-2.1.2.tgz" integrity sha512-t73xJJrvdTjXrn4jLS9VSGRbz0nUY3cl2DMGDU48lKl+HR9dbbjW2A9r3g40VA++mQpy6uuHg33gy7du2BKpog== +"@types/cors@^2.8.12": + version "2.8.12" + resolved "https://registry.yarnpkg.com/@types/cors/-/cors-2.8.12.tgz#6b2c510a7ad7039e98e7b8d3d6598f4359e5c080" + integrity sha512-vt+kDhq/M2ayberEtJcIN/hxXy1Pk+59g2FV/ZQceeaTyCtCucjL2Q7FXlFjtWn4n15KCr1NE2lNNFhp0lEThw== + "@types/express-serve-static-core@^4.17.18": version "4.17.24" resolved "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-4.17.24.tgz"