Skip to content

Commit

Permalink
Use cors module instead of custom cors logic (#2823)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
mifi and juliangruber authored Mar 26, 2021
1 parent 6c4f79b commit 415681f
Show file tree
Hide file tree
Showing 9 changed files with 150 additions and 88 deletions.
1 change: 0 additions & 1 deletion examples/custom-provider/server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/@uppy/companion/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
30 changes: 3 additions & 27 deletions packages/@uppy/companion/src/companion.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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))
Expand Down
44 changes: 36 additions & 8 deletions packages/@uppy/companion/src/server/middlewares.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
const uniq = require('lodash/uniq')
const cors = require('cors')

const tokenService = require('./helpers/jwt')
const logger = require('./logger')

Expand Down Expand Up @@ -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)
}
36 changes: 11 additions & 25 deletions packages/@uppy/companion/src/standalone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const { version } = require('../../package.json')
*
* @returns {object}
*/
function server (moreCompanionOptions = {}) {
function server (inputCompanionOptions = {}) {
const app = express()

// for server metrics tracking.
Expand Down Expand Up @@ -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,
Expand All @@ -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'
Expand Down
93 changes: 93 additions & 0 deletions packages/@uppy/companion/test/__tests__/cors.js
Original file line number Diff line number Diff line change
@@ -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'],
])
})
})
26 changes: 0 additions & 26 deletions packages/@uppy/companion/test/__tests__/middlewares.js

This file was deleted.

6 changes: 5 additions & 1 deletion website/src/docs/companion.md
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 415681f

Please sign in to comment.