Use cors module instead of custom cors logic (#2823)

* use cors module instead of custom cors logic #2762

- adds support for regex COMPANION_CLIENT_ORIGINS_REGEX
- encapsulate custom cors header merge logic in own middleware
- pull out non cors logic from middleware
- unit test the cors middleware

* fix capitalization

Co-authored-by: Julian Gruber <julian@juliangruber.com>

Co-authored-by: Julian Gruber <julian@juliangruber.com>

examples/custom-provider/server/index.js

@@ -15,7 +15,6 @@ app.use(session({
 }))
 
 app.use((req, res, next) => {
-  res.setHeader('Access-Control-Allow-Origin', req.headers.origin || '*')
   res.setHeader(
     'Access-Control-Allow-Methods',
     'GET, POST, OPTIONS, PUT, PATCH, DELETE'

package-lock.json

@@ -8748,6 +8748,7 @@
         "common-tags": "1.8.0",
         "connect-redis": "4.0.3",
         "cookie-parser": "1.4.5",
+        "cors": "^2.8.5",
         "escape-string-regexp": "2.0.0",
         "express": "4.17.1",
         "express-interceptor": "1.2.0",

packages/@uppy/companion/package.json

@@ -38,6 +38,7 @@
     "common-tags": "1.8.0",
     "connect-redis": "4.0.3",
     "cookie-parser": "1.4.5",
+    "cors": "^2.8.5",
     "escape-string-regexp": "2.0.0",
     "express": "4.17.1",
     "express-interceptor": "1.2.0",

packages/@uppy/companion/src/companion.js

@@ -6,6 +6,7 @@ const Grant = require('grant').express()
 const merge = require('lodash/merge')
 const cookieParser = require('cookie-parser')
 const interceptor = require('express-interceptor')
+
 const grantConfig = require('./config/grant')()
 const providerManager = require('./server/provider')
 const controllers = require('./server/controllers')
@@ -79,41 +80,16 @@ module.exports.app = (options = {}) => {
   app.use('/connect/:authProvider/:override?', getCredentialsOverrideMiddleware(providers, options))
   app.use(Grant(grantConfig))
 
-  app.use(middlewares.mergeAccessControlAllowMethods)
-
   app.use((req, res, next) => {
-    res.header(
-      'Access-Control-Allow-Headers',
-      [
-        'uppy-auth-token',
-        'uppy-versions',
-        'uppy-credentials-params',
-        res.get('Access-Control-Allow-Headers'),
-      ].join(',')
-    )
-
-    const exposedHeaders = [
-      // exposed so it can be accessed for our custom uppy preflight
-      'Access-Control-Allow-Headers',
-    ]
-
     if (options.sendSelfEndpoint) {
-      // add it to the exposed headers.
-      exposedHeaders.push('i-am')
-
       const { protocol } = options.server
       res.header('i-am', `${protocol}://${options.sendSelfEndpoint}`)
     }
-
-    if (res.get('Access-Control-Expose-Headers')) {
-      // if the header had been previously set, the values should be added too
-      exposedHeaders.push(res.get('Access-Control-Expose-Headers'))
-    }
-
-    res.header('Access-Control-Expose-Headers', exposedHeaders.join(','))
     next()
   })
 
+  app.use(middlewares.cors(options))
+
   // add uppy options to the request object so it can be accessed by subsequent handlers.
   app.use('*', getOptionsMiddleware(options))
   app.use('/s3', s3(options.providerOptions.s3))

packages/@uppy/companion/src/server/middlewares.js

@@ -1,4 +1,6 @@
 const uniq = require('lodash/uniq')
+const cors = require('cors')
+
 const tokenService = require('./helpers/jwt')
 const logger = require('./logger')
 
@@ -73,15 +75,41 @@ exports.loadSearchProviderToken = (req, res, next) => {
   next()
 }
 
-exports.mergeAccessControlAllowMethods = (req, res, next) => {
-  const existingHeader = res.get('Access-Control-Allow-Methods')
-  let existingMethods = []
-  if (existingHeader) {
-    existingMethods = existingHeader.replace(/\s/g, '').split(',').map((method) => method.toUpperCase())
+exports.cors = (options = {}) => (req, res, next) => {
+  const exposedHeaders = [
+    // exposed so it can be accessed for our custom uppy client preflight
+    'Access-Control-Allow-Headers',
+  ]
+  if (options.sendSelfEndpoint) exposedHeaders.push('i-am')
+  if (res.get('Access-Control-Expose-Headers')) exposedHeaders.push(res.get('Access-Control-Expose-Headers'))
+
+  const allowedHeaders = [
+    'uppy-auth-token',
+    'uppy-versions',
+    'uppy-credentials-params',
+  ]
+  if (res.get('Access-Control-Allow-Headers')) allowedHeaders.push(res.get('Access-Control-Allow-Headers'))
+
+  const existingAllowMethodsHeader = res.get('Access-Control-Allow-Methods')
+  let methods = []
+  if (existingAllowMethodsHeader) {
+    methods = existingAllowMethodsHeader.replace(/\s/g, '').split(',').map((method) => method.toUpperCase())
   }
+  methods = uniq([...methods, 'GET', 'POST', 'OPTIONS', 'DELETE'])
 
-  const mergedMethods = uniq([...existingMethods, 'GET', 'POST', 'OPTIONS', 'DELETE'])
+  // If endpoint urls are specified, then we only allow those endpoints.
+  // Otherwise, we allow any client url to access companion.
+  // Must be set to at least true (origin "*" with "credentials: true" will cause error in many browsers)
+  // https://github.com/expressjs/cors/issues/119
+  // allowedOrigins can also be any type supported by https://github.com/expressjs/cors#configuration-options
+  const { corsOrigins: origin = true } = options
 
-  res.header('Access-Control-Allow-Methods', mergedMethods.join(','))
-  next()
+  // Because we need to merge with existing headers, we need to call cors inside our own middleware
+  return cors({
+    credentials: true,
+    origin,
+    methods,
+    allowedHeaders: allowedHeaders.join(','),
+    exposedHeaders: exposedHeaders.join(','),
+  })(req, res, next)
 }

packages/@uppy/companion/src/standalone/index.js

@@ -21,7 +21,7 @@ const { version } = require('../../package.json')
  *
  * @returns {object}
  */
-function server (moreCompanionOptions = {}) {
+function server (inputCompanionOptions = {}) {
   const app = express()
 
   // for server metrics tracking.
@@ -108,6 +108,16 @@ function server (moreCompanionOptions = {}) {
   app.use(helmet.ieNoOpen())
   app.disable('x-powered-by')
 
+  let corsOrigins
+  if (process.env.COMPANION_CLIENT_ORIGINS) {
+    corsOrigins = process.env.COMPANION_CLIENT_ORIGINS
+      .split(',')
+      .map((url) => (helper.hasProtocol(url) ? url : `${process.env.COMPANION_PROTOCOL || 'http'}://${url}`))
+  } else if (process.env.COMPANION_CLIENT_ORIGINS_REGEX) {
+    corsOrigins = new RegExp(process.env.COMPANION_CLIENT_ORIGINS_REGEX)
+  }
+
+  const moreCompanionOptions = { ...inputCompanionOptions, corsOrigins }
   const companionOptions = helper.getCompanionOptions(moreCompanionOptions)
   const sessionOptions = {
     secret: companionOptions.secret,
@@ -133,30 +143,6 @@ function server (moreCompanionOptions = {}) {
   app.use(session(sessionOptions))
 
   app.use((req, res, next) => {
-    const protocol = process.env.COMPANION_PROTOCOL || 'http'
-
-    // if endpoint urls are specified, then we only allow those endpoints
-    // otherwise, we allow any client url to access companion.
-    // here we also enforce that only the protocol allowed by companion is used.
-    if (process.env.COMPANION_CLIENT_ORIGINS) {
-      const whitelist = process.env.COMPANION_CLIENT_ORIGINS
-        .split(',')
-        .map((url) => (helper.hasProtocol(url) ? url : `${protocol}://${url}`))
-
-      // @ts-ignore
-      if (req.headers.origin && whitelist.indexOf(req.headers.origin) > -1) {
-        res.setHeader('Access-Control-Allow-Origin', req.headers.origin)
-        // only allow credentials when origin is whitelisted
-        res.setHeader('Access-Control-Allow-Credentials', 'true')
-      }
-    } else {
-      res.setHeader('Access-Control-Allow-Origin', req.headers.origin || '*')
-    }
-
-    res.setHeader(
-      'Access-Control-Allow-Methods',
-      'GET, POST, OPTIONS, PUT, PATCH, DELETE'
-    )
     res.setHeader(
       'Access-Control-Allow-Headers',
       'Authorization, Origin, Content-Type, Accept'

packages/@uppy/companion/test/__tests__/cors.js

@@ -0,0 +1,93 @@
+/* global jest:false, test:false, describe:false, expect:false */
+
+const { cors } = require('../../src/server/middlewares')
+
+function testWithMock ({ corsOptions, get = () => {}, origin = 'https://localhost:1234' } = {}) {
+  const res = {
+    get,
+    getHeader: get,
+    setHeader: jest.fn(),
+    end: jest.fn(),
+  }
+  const req = {
+    method: 'OPTIONS',
+    headers: {
+      origin,
+    },
+  }
+  const next = jest.fn()
+  cors(corsOptions)(req, res, next)
+  return { res }
+}
+
+describe('cors', () => {
+  test('should properly merge with existing headers', () => {
+    const get = (header) => {
+      if (header.toLowerCase() === 'access-control-allow-methods') return 'PATCH,OPTIONS, post'
+      if (header.toLowerCase() === 'access-control-allow-headers') return 'test-allow-header'
+      if (header.toLowerCase() === 'access-control-expose-headers') return 'test'
+      return undefined
+    }
+
+    const { res } = testWithMock({
+      corsOptions: {
+        sendSelfEndpoint: true,
+        corsOrigins: /^https:\/\/localhost:.*$/,
+      },
+      get,
+    })
+    expect(res.setHeader.mock.calls).toEqual([
+      ['Access-Control-Allow-Origin', 'https://localhost:1234'],
+      ['Vary', 'Origin'],
+      ['Access-Control-Allow-Credentials', 'true'],
+      ['Access-Control-Allow-Methods', 'PATCH,OPTIONS,POST,GET,DELETE'],
+      ['Access-Control-Allow-Headers', 'uppy-auth-token,uppy-versions,uppy-credentials-params,test-allow-header'],
+      ['Access-Control-Expose-Headers', 'Access-Control-Allow-Headers,i-am,test'],
+      ['Content-Length', '0'],
+    ])
+    // expect(next).toHaveBeenCalled()
+  })
+
+  test('should also work when nothing added', () => {
+    const { res } = testWithMock()
+    expect(res.setHeader.mock.calls).toEqual([
+      ['Access-Control-Allow-Origin', 'https://localhost:1234'],
+      ['Vary', 'Origin'],
+      ['Access-Control-Allow-Credentials', 'true'],
+      ['Access-Control-Allow-Methods', 'GET,POST,OPTIONS,DELETE'],
+      ['Access-Control-Allow-Headers', 'uppy-auth-token,uppy-versions,uppy-credentials-params'],
+      ['Access-Control-Expose-Headers', 'Access-Control-Allow-Headers'],
+      ['Content-Length', '0'],
+    ])
+  })
+
+  test('should support disabling cors', () => {
+    const { res } = testWithMock({ corsOptions: { corsOrigins: false } })
+    expect(res.setHeader.mock.calls).toEqual([])
+  })
+
+  test('should support incorrect url', () => {
+    const { res } = testWithMock({ corsOptions: { corsOrigins: /^incorrect$/ } })
+    expect(res.setHeader.mock.calls).toEqual([
+      ['Vary', 'Origin'],
+      ['Access-Control-Allow-Credentials', 'true'],
+      ['Access-Control-Allow-Methods', 'GET,POST,OPTIONS,DELETE'],
+      ['Access-Control-Allow-Headers', 'uppy-auth-token,uppy-versions,uppy-credentials-params'],
+      ['Access-Control-Expose-Headers', 'Access-Control-Allow-Headers'],
+      ['Content-Length', '0'],
+    ])
+  })
+
+  test('should support array origin', () => {
+    const { res } = testWithMock({ corsOptions: { corsOrigins: ['http://google.com', 'https://localhost:1234'] } })
+    expect(res.setHeader.mock.calls).toEqual([
+      ['Access-Control-Allow-Origin', 'https://localhost:1234'],
+      ['Vary', 'Origin'],
+      ['Access-Control-Allow-Credentials', 'true'],
+      ['Access-Control-Allow-Methods', 'GET,POST,OPTIONS,DELETE'],
+      ['Access-Control-Allow-Headers', 'uppy-auth-token,uppy-versions,uppy-credentials-params'],
+      ['Access-Control-Expose-Headers', 'Access-Control-Allow-Headers'],
+      ['Content-Length', '0'],
+    ])
+  })
+})

packages/@uppy/companion/test/__tests__/middlewares.js

@@ -1,26 +0,0 @@
-/* global jest:false, test:false, describe:false, expect:false */
-
-const { mergeAccessControlAllowMethods } = require('../../src/server/middlewares')
-
-describe('mergeAccessControlAllowMethods', () => {
-  test('should properly merge', () => {
-    const res = {
-      get: () => 'PATCH,OPTIONS, post',
-      header: jest.fn(),
-    }
-    const next = jest.fn()
-    mergeAccessControlAllowMethods(undefined, res, next)
-    expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Methods', 'PATCH,OPTIONS,POST,GET,DELETE')
-    expect(next).toHaveBeenCalled()
-  })
-  test('should also work when nothing added', () => {
-    const res = {
-      get: () => undefined,
-      header: jest.fn(),
-    }
-    const next = jest.fn()
-    mergeAccessControlAllowMethods(undefined, res, next)
-    expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Methods', 'GET,POST,OPTIONS,DELETE')
-    expect(next).toHaveBeenCalled()
-  })
-})

website/src/docs/companion.md

@@ -162,9 +162,13 @@ export COMPANION_HIDE_METRICS="true"
 export COMPANION_IMPLICIT_PATH="/SERVER/PATH/TO/WHERE/UPPY/SERVER/LIVES"
 
 # comma-separated client hosts to whitlelist by the server
-# if not specified, the server would allow any host
+# if neither this or COMPANION_CLIENT_ORIGINS_REGEX specified, the server would allow any host
 export COMPANION_CLIENT_ORIGINS="localhost:3452,uppy.io"
 
+# Like COMPANION_CLIENT_ORIGINS, but allows a single regex instead
+# (COMPANION_CLIENT_ORIGINS will be ignored if this is used and vice versa)
+export COMPANION_CLIENT_ORIGINS_REGEX="https://.*\.example\.(com|eu)$"
+
 # corresponds to the redisUrl option
 # this also enables Redis session storage if set
 export COMPANION_REDIS_URL="REDIS URL"