Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

companion,companion-client: send uppy-versions header to companion #1612

Merged
merged 7 commits into from
Jun 26, 2019
439 changes: 230 additions & 209 deletions package-lock.json

Large diffs are not rendered by default.

61 changes: 53 additions & 8 deletions packages/@uppy/companion-client/src/RequestClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ module.exports = class RequestClient {
this.uppy = uppy
this.opts = opts
this.onReceiveResponse = this.onReceiveResponse.bind(this)
this.allowedHeaders = ['accept', 'content-type', 'uppy-auth-token']
this.preflightDone = false
}

get hostname () {
Expand All @@ -25,12 +27,15 @@ module.exports = class RequestClient {
get defaultHeaders () {
return {
'Accept': 'application/json',
'Content-Type': 'application/json'
'Content-Type': 'application/json',
'Uppy-Versions': '@uppy/companion-client=1.0.3'
goto-bus-stop marked this conversation as resolved.
Show resolved Hide resolved
}
}

headers () {
return Promise.resolve(Object.assign({}, this.defaultHeaders, this.opts.serverHeaders || {}))
return Promise.resolve(
Object.assign({}, this.defaultHeaders, this.opts.serverHeaders || {})
)
}

_getPostResponseFunc (skip) {
Expand Down Expand Up @@ -77,9 +82,49 @@ module.exports = class RequestClient {
return res.json()
}

preflight (path) {
return new Promise((resolve, reject) => {
if (this.preflightDone) {
return resolve(this.allowedHeaders.slice())
}

fetch(this._getUrl(path), {
method: 'OPTIONS'
Copy link
Contributor

@goto-bus-stop goto-bus-stop Jun 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually getting an error on this because OPTIONS is not in the CORS whitelist …
image

Request headers:
image

Response headers:
image

If this is the case we may need a new separate endpoint that we can just GET after all :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unable to reproduce the issue locally with firefox. We've always allowed all methods for all endpoints

Are you using the companion standalone server when testing? Also are you able to reproduce this with other providers asides S3? Here's my network log when testing with instagram on firefox

Screenshot 2019-06-18 at 11 39 30 AM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did renee test it against transloadit? Can you? Maybe we're messing something up over there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh…I tested using the examples/aws-companion example in this repo, which does not use the standalone server, and uses the default configuration of the cors module. It doesn't do this thing: https://github.com/expressjs/cors#enabling-cors-pre-flight

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did renee test it against transloadit? Can you? Maybe we're messing something up over there?

Transloadit directly uses the standalone server so I wouldn't think we would be messing anything up there.

Ahh…I tested using the examples/aws-companion example in this repo, which does not use the standalone server

Ah alright, it makes sense now.

Users who have their own personalized express servers are left to determine what methods they allow on their servers, but maybe we should change this and let the companion pluggable app set the allowed methods for its own endpoints 🤔

Also, should this still be considered a breaking change if it breaks only for users who don't use the standalone server, and happen not to allow the OPTIONS method on their server? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the entire upload fails right now if the preflight request fails, maybe we can change it so the preflight request failure is caught, and we use the default allowedHeaders in that case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transloadit directly uses the standalone server so I wouldn't think we would be messing anything up there.

That's true but it does pass through HAProxy which does/can tamper with headers. Not the issue right now it seems, but I thought i'd add that as a friendly reminder here to save us a possible headache later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true but it does pass through HAProxy which does/can tamper with headers

Ah ok, I thought you once mentioned that it does almost nothing (for the case of companion) but forward requests to companion, so I was working with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can change it so the preflight request failure is caught, and we use the default allowedHeaders in that case?

will do 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goto-bus-stop I have made an update.

PS: I had to force push because I did a rebase to resolve conflicts

})
.then((response) => {
if (response.headers.has('access-control-allow-headers')) {
this.allowedHeaders = response.headers.get('access-control-allow-headers')
.split(',').map((headerName) => headerName.trim().toLowerCase())
}
this.preflightDone = true
resolve(this.allowedHeaders.slice())
})
.catch((err) => {
this.uppy.log(`[CompanionClient] unable to make preflight request ${err}`, 'warning')
this.preflightDone = true
resolve(this.allowedHeaders.slice())
})
})
}

preflightAndHeaders (path) {
return Promise.all([this.preflight(path), this.headers()])
.then(([allowedHeaders, headers]) => {
// filter to keep only allowed Headers
Object.keys(headers).forEach((header) => {
if (allowedHeaders.indexOf(header.toLowerCase()) === -1) {
this.uppy.log(`[CompanionClient] excluding unallowed header ${header}`)
delete headers[header]
}
})

return headers
})
}

get (path, skipPostResponse) {
return new Promise((resolve, reject) => {
this.headers().then((headers) => {
this.preflightAndHeaders(path).then((headers) => {
fetch(this._getUrl(path), {
method: 'get',
headers: headers,
Expand All @@ -91,13 +136,13 @@ module.exports = class RequestClient {
err = err.isAuthError ? err : new Error(`Could not get ${this._getUrl(path)}. ${err}`)
reject(err)
})
})
}).catch(reject)
})
}

post (path, data, skipPostResponse) {
return new Promise((resolve, reject) => {
this.headers().then((headers) => {
this.preflightAndHeaders(path).then((headers) => {
fetch(this._getUrl(path), {
method: 'post',
headers: headers,
Expand All @@ -110,13 +155,13 @@ module.exports = class RequestClient {
err = err.isAuthError ? err : new Error(`Could not post ${this._getUrl(path)}. ${err}`)
reject(err)
})
})
}).catch(reject)
})
}

delete (path, data, skipPostResponse) {
return new Promise((resolve, reject) => {
this.headers().then((headers) => {
this.preflightAndHeaders(path).then((headers) => {
fetch(`${this.hostname}/${path}`, {
method: 'delete',
headers: headers,
Expand All @@ -129,7 +174,7 @@ module.exports = class RequestClient {
err = err.isAuthError ? err : new Error(`Could not delete ${this._getUrl(path)}. ${err}`)
reject(err)
})
})
}).catch(reject)
})
}
}
5 changes: 5 additions & 0 deletions packages/@uppy/companion/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 @@ -56,6 +56,7 @@
"purest": "3.0.0",
"redis": "2.8.0",
"request": "2.85.0",
"semver": "6.1.1",
"serialize-error": "^2.1.0",
"tus-js-client": "^1.8.0-0",
"uuid": "2.0.2",
Expand Down
5 changes: 3 additions & 2 deletions packages/@uppy/companion/src/server/controllers/send-token.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const tokenService = require('../helpers/jwt')
const parseUrl = require('url').parse // eslint-disable-line node/no-deprecated-api
const { hasMatch, sanitizeHtml } = require('../helpers/utils')
const oAuthState = require('../helpers/oauth-state')
const versionCmp = require('../helpers/version')

/**
*
Expand All @@ -29,8 +30,8 @@ module.exports = function sendToken (req, res, next) {
const allowedClients = req.uppy.options.clients
// if no preset clients then allow any client
if (!allowedClients || hasMatch(origin, allowedClients) || hasMatch(parseUrl(origin).host, allowedClients)) {
// @todo do a more secure client version check, see https://www.npmjs.com/package/semver
return res.send(clientVersion ? htmlContent(uppyAuthToken, origin) : oldHtmlContent(uppyAuthToken, origin))
const allowsStringMessage = versionCmp.gte(clientVersion, '1.0.2')
ifedapoolarewaju marked this conversation as resolved.
Show resolved Hide resolved
return res.send(allowsStringMessage ? htmlContent(uppyAuthToken, origin) : oldHtmlContent(uppyAuthToken, origin))
}
}
next()
Expand Down
14 changes: 14 additions & 0 deletions packages/@uppy/companion/src/server/helpers/version.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const semver = require('semver')

/**
* checks if a version is greater than or equal to
* @param {string} v1 the LHS version
* @param {string} v2 the RHS version
* @returns {boolean}
*/
exports.gte = (v1, v2) => {
v1 = semver.coerce(v1).version
v2 = semver.coerce(v2).version

return semver.gte(v1, v2)
}
1 change: 1 addition & 0 deletions packages/@uppy/companion/src/standalone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const promInterval = collectDefaultMetrics({ register: promClient.register, time

// Add version as a prometheus gauge
const versionGauge = new promClient.Gauge({ name: 'companion_version', help: 'npm version as an integer' })
// @ts-ignore
const numberVersion = version.replace(/\D/g, '') * 1
versionGauge.set(numberVersion)

Expand Down
38 changes: 27 additions & 11 deletions packages/@uppy/companion/src/uppy.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,34 @@ module.exports.app = (options = {}) => {
app.use((req, res, next) => {
res.header(
'Access-Control-Allow-Headers',
[res.get('Access-Control-Allow-Headers'), 'uppy-auth-token', 'uppy-client'].join(', ')
[
'uppy-auth-token',
'uppy-versions',
res.get('Access-Control-Allow-Headers')
].join(',')
)
next()
})
if (options.sendSelfEndpoint) {
app.use('*', (req, res, next) => {

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}`)
// add it to the exposed custom headers.
res.header('Access-Control-Expose-Headers', [res.get('Access-Control-Expose-Headers'), 'i-am'].join(', '))
next()
})
}
}

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()
})

// add uppy options to the request object so it can be accessed by subsequent handlers.
app.use('*', getOptionsMiddleware(options))
Expand Down Expand Up @@ -214,11 +229,12 @@ const getOptionsMiddleware = (options) => {
* @param {function} next
*/
const middleware = (req, res, next) => {
const versionFromQuery = req.query.uppyVersions ? decodeURIComponent(req.query.uppyVersions) : null
req.uppy = {
options,
s3Client,
authToken: req.header('uppy-auth-token') || req.query.uppyAuthToken,
clientVersion: req.header('uppy-versions') || req.query.uppyVersions,
clientVersion: req.header('uppy-versions') || versionFromQuery || '1.0.0',
buildURL: getURLBuilder(options)
}
next()
Expand Down
68 changes: 64 additions & 4 deletions packages/@uppy/companion/test/__tests__/companion.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,24 @@ jest.mock('../../src/server/helpers/oauth-state', () => {
return {
generateState: () => 'some-cool-nice-encrytpion',
addToState: () => 'some-cool-nice-encrytpion',
getFromState: (state) => {
return state === 'state-with-invalid-instance-url' ? 'http://localhost:3452' : 'http://localhost:3020'
getFromState: (state, key) => {
if (state === 'state-with-invalid-instance-url') {
return 'http://localhost:3452'
}

if (state === 'state-with-older-version' && key === 'clientVersion') {
return '@uppy/companion-client=1.0.1'
}

if (state === 'state-with-newer-version' && key === 'clientVersion') {
return '@uppy/companion-client=1.0.3'
}

if (state === 'state-with-newer-version-old-style' && key === 'clientVersion') {
return 'companion-client:1.0.2'
}

return 'http://localhost:3020'
}
}
})
Expand Down Expand Up @@ -76,13 +92,57 @@ describe('test authentication', () => {
})

test('the token gets sent via cookie and html', () => {
// see mock ../../src/server/helpers/oauth-state above for state values
return request(authServer)
.get(`/drive/send-token?uppyAuthToken=${token}`)
.get(`/drive/send-token?uppyAuthToken=${token}&state=state-with-newer-version`)
.expect(200)
.expect((res) => {
const authToken = res.header['set-cookie'][0].split(';')[0].split('uppyAuthToken--google=')[1]
expect(authToken).toEqual(token)
// see mock ../../src/server/helpers/oauth-state above for http://localhost:3020
const body = `
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<script>
window.opener.postMessage(JSON.stringify({token: "${token}"}), "http://localhost:3020")
window.close()
</script>
</head>
<body></body>
</html>`
expect(res.text).toBe(body)
})
})

test('the token gets to older clients without stringify', () => {
// see mock ../../src/server/helpers/oauth-state above for state values
return request(authServer)
.get(`/drive/send-token?uppyAuthToken=${token}&state=state-with-older-version`)
.expect(200)
.expect((res) => {
const body = `
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<script>
window.opener.postMessage({token: "${token}"}, "http://localhost:3020")
window.close()
</script>
</head>
<body></body>
</html>`
expect(res.text).toBe(body)
})
})

test('the token gets sent to newer clients with old version style', () => {
// see mock ../../src/server/helpers/oauth-state above for state values
return request(authServer)
.get(`/drive/send-token?uppyAuthToken=${token}&state=state-with-newer-version-old-style`)
.expect(200)
.expect((res) => {
const body = `
<!DOCTYPE html>
<html>
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/companion/test/mockserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ authServer.all('*/callback', (req, res, next) => {
})
authServer.all('/drive/send-token', (req, res, next) => {
req.session.grant = {
state: 'non-empty-value' }
state: req.query.state || 'non-empty-value' }
next()
})

Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/provider-views/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ module.exports = class ProviderView {
handleAuth () {
const authState = btoa(JSON.stringify({ origin: getOrigin() }))
// @todo remove this hardcoded version
const clientVersion = 'companion-client:1.0.2'
const clientVersion = encodeURIComponent('@uppy/companion-client=1.0.2')
goto-bus-stop marked this conversation as resolved.
Show resolved Hide resolved
const link = `${this.provider.authUrl()}?state=${authState}&uppyVersions=${clientVersion}`

const authWindow = window.open(link, '_blank')
Expand Down