diff --git a/package.json b/package.json index 77ffe8c41e..a51628d716 100644 --- a/package.json +++ b/package.json @@ -154,6 +154,7 @@ "e2e:headless": "yarn workspace e2e cypress:headless", "e2e:generate": "yarn workspace e2e generate-test", "test:companion": "yarn workspace @uppy/companion test", + "test:companion:watch": "yarn workspace @uppy/companion test --watch", "test:endtoend:local": "yarn workspace @uppy-tests/end2end test:endtoend:local", "test:endtoend": "yarn workspace @uppy-tests/end2end test:endtoend", "test:locale-packs": "yarn locale-packs:unused && yarn locale-packs:warnings", @@ -163,7 +164,7 @@ "test:unit": "yarn run build:lib && NODE_OPTIONS=--experimental-vm-modules jest --env jsdom", "test:watch": "jest --env jsdom --watch", "test:size": "yarn build:lib && size-limit --why", - "test": "npm-run-all lint test:locale-packs test:unit test:type test:companion", + "test": "npm-run-all lint test:locale-packs:unused test:locale-packs:warnings test:unit test:type test:companion", "uploadcdn": "yarn node ./bin/upload-to-cdn.js", "version": "yarn node ./bin/after-version-bump.js", "watch:css": "onchange 'packages/{@uppy/,}*/src/*.scss' --initial --verbose -- yarn run build:css", @@ -192,6 +193,9 @@ ], "testMatch": [ "**/packages/**/*.test.js" + ], + "testPathIgnorePatterns": [ + "/packages/@uppy/companion/" ] }, "resolutions": { diff --git a/packages/@uppy/companion/.gitignore b/packages/@uppy/companion/.gitignore index 9f07685ced..abc16a6eab 100644 --- a/packages/@uppy/companion/.gitignore +++ b/packages/@uppy/companion/.gitignore @@ -31,7 +31,6 @@ config/auth.js *.pem env.* -!env.test.sh output/* test/output/* diff --git a/packages/@uppy/companion/env.test.sh b/packages/@uppy/companion/env.test.sh deleted file mode 100644 index 840e39a181..0000000000 --- a/packages/@uppy/companion/env.test.sh +++ /dev/null @@ -1,28 +0,0 @@ -export NODE_ENV="test" -export COMPANION_PORT=3020 -export COMPANION_DOMAIN="localhost:3020" -export COMPANION_SELF_ENDPOINT="localhost:3020" -export COMPANION_HIDE_METRICS="false" -export COMPANION_HIDE_WELCOME="false" - -export COMPANION_STREAMING_UPLOAD="true" - -export COMPANION_PROTOCOL="http" -export COMPANION_DATADIR="./test/output" -export COMPANION_SECRET="secret" - -export COMPANION_DROPBOX_KEY="dropbox_key" -export COMPANION_DROPBOX_SECRET="dropbox_secret" - -export COMPANION_BOX_KEY="box_key" -export COMPANION_BOX_SECRET="box_secret" - -export COMPANION_GOOGLE_KEY="google_key" -export COMPANION_GOOGLE_SECRET="google_secret" - -export COMPANION_INSTAGRAM_KEY="instagram_key" -export COMPANION_INSTAGRAM_SECRET="instagram_secret" - -export COMPANION_ZOOM_KEY="zoom_key" -export COMPANION_ZOOM_SECRET="zoom_secret" -export COMPANION_ZOOM_VERIFICATION_TOKEN="zoom_verfication_token" diff --git a/packages/@uppy/companion/package.json b/packages/@uppy/companion/package.json index 1984066806..e1f661c9e7 100644 --- a/packages/@uppy/companion/package.json +++ b/packages/@uppy/companion/package.json @@ -110,8 +110,7 @@ "deploy": "kubectl apply -f infra/kube/companion-kube.yml", "prepublishOnly": "yarn run build", "start": "node ./lib/standalone/start-server.js", - "test": "bash -c 'source env.test.sh && ../../../node_modules/jest/bin/jest.js'", - "test:watch": "yarn test -- --watch" + "test": "jest" }, "engines": { "node": ">=10.20.1" diff --git a/packages/@uppy/companion/src/server/Uploader.js b/packages/@uppy/companion/src/server/Uploader.js index 259b752dad..c5018814f5 100644 --- a/packages/@uppy/companion/src/server/Uploader.js +++ b/packages/@uppy/companion/src/server/Uploader.js @@ -1,10 +1,10 @@ +// eslint-disable-next-line max-classes-per-file const tus = require('tus-js-client') const uuid = require('uuid') const isObject = require('isobject') const validator = require('validator') const request = require('request') -// eslint-disable-next-line no-unused-vars -const { Readable, pipeline: pipelineCb } = require('stream') +const { pipeline: pipelineCb } = require('stream') const { join } = require('path') const fs = require('fs') const { promisify } = require('util') @@ -40,8 +40,21 @@ function exceedsMaxFileSize (maxFileSize, size) { return maxFileSize && size && size > maxFileSize } +// TODO remove once we migrate away from form-data +function sanitizeMetadata (inputMetadata) { + if (inputMetadata == null) return {} + + const outputMetadata = {} + Object.keys(inputMetadata).forEach((key) => { + outputMetadata[key] = String(inputMetadata[key]) + }) + return outputMetadata +} + class AbortError extends Error {} +class ValidationError extends Error {} + class Uploader { /** * Uploads file to destination based on the supplied protocol (tus, s3-multipart, multipart) @@ -67,15 +80,12 @@ class Uploader { * @param {UploaderOptions} options */ constructor (options) { - if (!this.validateOptions(options)) { - logger.debug(this._errRespMessage, 'uploader.validator.fail') - return - } + this.validateOptions(options) this.options = options this.token = uuid.v4() this.fileName = `${Uploader.FILE_NAME_PREFIX}-${this.token}` - this.options.metadata = this.options.metadata || {} + this.options.metadata = sanitizeMetadata(this.options.metadata) this.options.fieldname = this.options.fieldname || DEFAULT_FIELD_NAME this.size = options.size this.uploadFileName = this.options.metadata.name @@ -173,7 +183,7 @@ class Uploader { /** * - * @param {Readable} stream + * @param {import('stream').Readable} stream */ async uploadStream (stream) { try { @@ -188,13 +198,30 @@ class Uploader { // The stream will then typically come from a "Transfer-Encoding: chunked" response await this._downloadStreamAsFile(this.readStream) } - if (this.uploadStopped) return + if (this.uploadStopped) return undefined const { url, extraData } = await Promise.race([ this._uploadByProtocol(), // If we don't handle stream errors, we get unhandled error in node. new Promise((resolve, reject) => this.readStream.on('error', reject)), ]) + return { url, extraData } + } finally { + logger.debug('cleanup', this.shortToken) + if (this.readStream && !this.readStream.destroyed) this.readStream.destroy() + if (this.tmpPath) unlink(this.tmpPath).catch(() => {}) + } + } + + /** + * + * @param {import('stream').Readable} stream + */ + async tryUploadStream (stream) { + try { + const ret = await this.uploadStream(stream) + if (!ret) return + const { url, extraData } = ret this.emitSuccess(url, extraData) } catch (err) { if (err instanceof AbortError) { @@ -205,7 +232,9 @@ class Uploader { logger.error(err, 'uploader.error', this.shortToken) this.emitError(err) } finally { - this.cleanUp() + emitter().removeAllListeners(`pause:${this.token}`) + emitter().removeAllListeners(`resume:${this.token}`) + emitter().removeAllListeners(`cancel:${this.token}`) } } @@ -250,51 +279,43 @@ class Uploader { * Validate the options passed down to the uplaoder * * @param {UploaderOptions} options - * @returns {boolean} */ validateOptions (options) { // validate HTTP Method if (options.httpMethod) { if (typeof options.httpMethod !== 'string') { - this._errRespMessage = 'unsupported HTTP METHOD specified' - return false + throw new ValidationError('unsupported HTTP METHOD specified') } const method = options.httpMethod.toLowerCase() if (method !== 'put' && method !== 'post') { - this._errRespMessage = 'unsupported HTTP METHOD specified' - return false + throw new ValidationError('unsupported HTTP METHOD specified') } } if (exceedsMaxFileSize(options.companionOptions.maxFileSize, options.size)) { - this._errRespMessage = 'maxFileSize exceeded' - return false + throw new ValidationError('maxFileSize exceeded') } // validate fieldname if (options.fieldname && typeof options.fieldname !== 'string') { - this._errRespMessage = 'fieldname must be a string' - return false + throw new ValidationError('fieldname must be a string') } // validate metadata - if (options.metadata && !isObject(options.metadata)) { - this._errRespMessage = 'metadata must be an object' - return false + if (options.metadata != null) { + if (!isObject(options.metadata)) throw new ValidationError('metadata must be an object') } // validate headers if (options.headers && !isObject(options.headers)) { - this._errRespMessage = 'headers must be an object' - return false + throw new ValidationError('headers must be an object') } // validate protocol // @todo this validation should not be conditional once the protocol field is mandatory if (options.protocol && !Object.keys(PROTOCOLS).some((key) => PROTOCOLS[key] === options.protocol)) { - this._errRespMessage = 'unsupported protocol specified' - return false + throw new ValidationError('unsupported protocol specified') } // s3 uploads don't require upload destination @@ -302,39 +323,27 @@ class Uploader { // by the server's s3 config if (options.protocol !== PROTOCOLS.s3Multipart) { if (!options.endpoint && !options.uploadUrl) { - this._errRespMessage = 'no destination specified' - return false + throw new ValidationError('no destination specified') } const validateUrl = (url) => { const validatorOpts = { require_protocol: true, require_tld: false } if (url && !validator.isURL(url, validatorOpts)) { - this._errRespMessage = 'invalid destination url' - return false + throw new ValidationError('invalid destination url') } const allowedUrls = options.companionOptions.uploadUrls if (allowedUrls && url && !hasMatch(url, allowedUrls)) { - this._errRespMessage = 'upload destination does not match any allowed destinations' - return false + throw new ValidationError('upload destination does not match any allowed destinations') } - - return true } - if (![options.endpoint, options.uploadUrl].every(validateUrl)) return false + [options.endpoint, options.uploadUrl].forEach(validateUrl) } if (options.chunkSize != null && typeof options.chunkSize !== 'number') { - this._errRespMessage = 'incorrect chunkSize' - return false + throw new ValidationError('incorrect chunkSize') } - - return true - } - - hasError () { - return this._errRespMessage != null } /** @@ -353,24 +362,6 @@ class Uploader { logger.debug('socket connection received', 'uploader.socket.wait', this.shortToken) } - cleanUp () { - logger.debug('cleanup', this.shortToken) - if (this.readStream && !this.readStream.destroyed) this.readStream.destroy() - - if (this.tmpPath) unlink(this.tmpPath).catch(() => {}) - - emitter().removeAllListeners(`pause:${this.token}`) - emitter().removeAllListeners(`resume:${this.token}`) - emitter().removeAllListeners(`cancel:${this.token}`) - } - - getResponse () { - if (this._errRespMessage) { - return { body: { message: this._errRespMessage }, status: 400 } - } - return { body: { token: this.token }, status: 200 } - } - /** * @typedef {{action: string, payload: object}} State * @param {State} state @@ -649,3 +640,4 @@ Uploader.FILE_NAME_PREFIX = 'uppy-file' Uploader.STORAGE_PREFIX = 'companion' module.exports = Uploader +module.exports.ValidationError = ValidationError diff --git a/packages/@uppy/companion/src/server/helpers/upload.js b/packages/@uppy/companion/src/server/helpers/upload.js index 5e2a45c162..ee45c79f82 100644 --- a/packages/@uppy/companion/src/server/helpers/upload.js +++ b/packages/@uppy/companion/src/server/helpers/upload.js @@ -2,6 +2,8 @@ const Uploader = require('../Uploader') const logger = require('../logger') const { errorToResponse } = require('../provider/error') +const { ValidationError } = Uploader + async function startDownUpload ({ req, res, getSize, download, onUnhandledError }) { try { const size = await getSize() @@ -9,12 +11,6 @@ async function startDownUpload ({ req, res, getSize, download, onUnhandledError logger.debug('Instantiating uploader.', null, req.id) const uploader = new Uploader(Uploader.reqToOptions(req, size)) - if (uploader.hasError()) { - const response = uploader.getResponse() - res.status(response.status).json(response.body) - return - } - const stream = await download() // "Forking" off the upload operation to background, so we can return the http request: @@ -25,14 +21,19 @@ async function startDownUpload ({ req, res, getSize, download, onUnhandledError await uploader.awaitReady() logger.debug('Socket connection received. Starting remote download/upload.', null, req.id) - await uploader.uploadStream(stream) + await uploader.tryUploadStream(stream) })().catch((err) => logger.error(err)) // Respond the request - // NOTE: Uploader will continue running after the http request is responded - const response = uploader.getResponse() - res.status(response.status).json(response.body) + // NOTE: the Uploader will continue running after the http request is responded + res.status(200).json({ token: uploader.token }) } catch (err) { + if (err instanceof ValidationError) { + logger.debug(err.message, 'uploader.validator.fail') + res.status(400).json({ message: err.message }) + return + } + const errResp = errorToResponse(err) if (errResp) { res.status(errResp.code).json({ message: errResp.message }) diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index fcae8d2c3e..ca8f9f31c7 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/packages/@uppy/companion/test/__tests__/provider-manager.js @@ -2,12 +2,14 @@ const providerManager = require('../../src/server/provider') const { getCompanionOptions } = require('../../src/standalone/helper') +const { setDefaultEnv } = require('../mockserver') let grantConfig let companionOptions describe('Test Provider options', () => { beforeEach(() => { + setDefaultEnv() grantConfig = require('../../src/config/grant')() companionOptions = getCompanionOptions() }) diff --git a/packages/@uppy/companion/test/__tests__/uploader.js b/packages/@uppy/companion/test/__tests__/uploader.js index 6ba32c07ef..2d1bd66fa8 100644 --- a/packages/@uppy/companion/test/__tests__/uploader.js +++ b/packages/@uppy/companion/test/__tests__/uploader.js @@ -4,11 +4,19 @@ jest.mock('tus-js-client') const intoStream = require('into-stream') const fs = require('fs') +const nock = require('nock') const Uploader = require('../../src/server/Uploader') const socketClient = require('../mocksocket') const standalone = require('../../src/standalone') +afterAll(() => { + nock.cleanAll() + nock.restore() +}) + +process.env.COMPANION_DATADIR = './test/output' +process.env.COMPANION_DOMAIN = 'localhost:3020' const { companionOptions } = standalone() describe('uploader with tus protocol', () => { @@ -18,7 +26,7 @@ describe('uploader with tus protocol', () => { companionOptions: { ...companionOptions, uploadUrls: [/^http:\/\/url.myendpoint.com\//] }, } - expect(new Uploader(opts).hasError()).toBe(true) + expect(() => new Uploader(opts)).toThrow(new Uploader.ValidationError('upload destination does not match any allowed destinations')) }) test('uploader respects uploadUrls, valid', async () => { @@ -27,7 +35,8 @@ describe('uploader with tus protocol', () => { companionOptions: { ...companionOptions, uploadUrls: [/^http:\/\/url.myendpoint.com\//] }, } - expect(new Uploader(opts).hasError()).toBe(false) + // eslint-disable-next-line no-new + new Uploader(opts) // no validation error }) test('uploader respects uploadUrls, localhost', async () => { @@ -36,7 +45,8 @@ describe('uploader with tus protocol', () => { companionOptions: { ...companionOptions, uploadUrls: [/^http:\/\/localhost:1337\//] }, } - expect(new Uploader(opts).hasError()).toBe(false) + // eslint-disable-next-line no-new + new Uploader(opts) // no validation error }) test('upload functions with tus protocol', async () => { @@ -52,13 +62,12 @@ describe('uploader with tus protocol', () => { const uploader = new Uploader(opts) const uploadToken = uploader.token - expect(uploader.hasError()).toBe(false) expect(uploadToken).toBeTruthy() return new Promise((resolve, reject) => { // validate that the test is resolved on socket connection uploader.awaitReady().then(() => { - uploader.uploadStream(stream).then(() => resolve()) + uploader.tryUploadStream(stream).then(() => resolve()) }) let progressReceived = 0 @@ -97,13 +106,12 @@ describe('uploader with tus protocol', () => { const uploader = new Uploader(opts) const uploadToken = uploader.token - expect(uploader.hasError()).toBe(false) expect(uploadToken).toBeTruthy() return new Promise((resolve, reject) => { // validate that the test is resolved on socket connection uploader.awaitReady().then(() => { - uploader.uploadStream(stream).then(() => { + uploader.tryUploadStream(stream).then(() => { try { expect(fs.existsSync(uploader.path)).toBe(false) resolve() @@ -143,6 +151,76 @@ describe('uploader with tus protocol', () => { }) }) + async function runMultipartTest ({ metadata, useFormData, includeSize = true } = {}) { + const fileContent = Buffer.from('Some file content') + const stream = intoStream(fileContent) + + const opts = { + companionOptions, + endpoint: 'http://localhost', + protocol: 'multipart', + size: includeSize ? fileContent.length : undefined, + metadata, + pathPrefix: companionOptions.filePath, + useFormData, + } + + const uploader = new Uploader(opts) + return uploader.uploadStream(stream) + } + + test('upload functions with xhr protocol', async () => { + nock('http://localhost').post('/').reply(200) + + const ret = await runMultipartTest() + expect(ret).toMatchObject({ url: null, extraData: { response: expect.anything(), bytesUploaded: 17 } }) + }) + + // eslint-disable-next-line max-len + const formDataNoMetaMatch = /^----------------------------\d+\r\nContent-Disposition: form-data; name="files\[\]"; filename="uppy-file-[^"]+"\r\nContent-Type: application\/octet-stream\r\n\r\nSome file content\r\n----------------------------\d+--\r\n$/ + + test('upload functions with xhr formdata', async () => { + nock('http://localhost').post('/', formDataNoMetaMatch) + .reply(200) + + const ret = await runMultipartTest({ useFormData: true }) + expect(ret).toMatchObject({ url: null, extraData: { response: expect.anything(), bytesUploaded: 17 } }) + }) + + test('upload functions with unknown file size', async () => { + // eslint-disable-next-line max-len + nock('http://localhost').post('/', formDataNoMetaMatch) + .reply(200) + + const ret = await runMultipartTest({ useFormData: true, includeSize: false }) + expect(ret).toMatchObject({ url: null, extraData: { response: expect.anything(), bytesUploaded: 17 } }) + }) + + // https://github.com/transloadit/uppy/issues/3477 + test('upload functions with xhr formdata and metadata', async () => { + // eslint-disable-next-line max-len + nock('http://localhost').post('/', /^----------------------------\d+\r\nContent-Disposition: form-data; name="key1"\r\n\r\nnull\r\n----------------------------\d+\r\nContent-Disposition: form-data; name="key2"\r\n\r\ntrue\r\n----------------------------\d+\r\nContent-Disposition: form-data; name="key3"\r\n\r\n\d+\r\n----------------------------\d+\r\nContent-Disposition: form-data; name="key4"\r\n\r\n\[object Object\]\r\n----------------------------\d+\r\nContent-Disposition: form-data; name="key5"\r\n\r\n\(\) => {}\r\n----------------------------\d+\r\nContent-Disposition: form-data; name="key6"\r\n\r\nSymbol\(\)\r\n----------------------------\d+\r\nContent-Disposition: form-data; name="files\[\]"; filename="uppy-file-[^"]+"\r\nContent-Type: application\/octet-stream\r\n\r\nSome file content\r\n----------------------------\d+--\r\n$/) + .reply(200) + + const metadata = { + key1: null, key2: true, key3: 1234, key4: {}, key5: () => {}, key6: Symbol(''), + } + const ret = await runMultipartTest({ useFormData: true, metadata }) + expect(ret).toMatchObject({ url: null, extraData: { response: expect.anything(), bytesUploaded: 17 } }) + }) + + test('uploader checks metadata', () => { + const opts = { + companionOptions, + endpoint: 'http://localhost', + } + + // eslint-disable-next-line no-new + new Uploader({ ...opts, metadata: { key: 'string value' } }) + + expect(() => new Uploader({ ...opts, metadata: '' })).toThrow(new Uploader.ValidationError('metadata must be an object')) + }) + test('uploader respects maxFileSize', async () => { const opts = { endpoint: 'http://url.myendpoint.com/files', @@ -150,8 +228,7 @@ describe('uploader with tus protocol', () => { size: 101, } - const uploader = new Uploader(opts) - expect(uploader.hasError()).toBe(true) + expect(() => new Uploader(opts)).toThrow(new Uploader.ValidationError('maxFileSize exceeded')) }) test('uploader respects maxFileSize correctly', async () => { @@ -161,8 +238,8 @@ describe('uploader with tus protocol', () => { size: 99, } - const uploader = new Uploader(opts) - expect(uploader.hasError()).toBe(false) + // eslint-disable-next-line no-new + new Uploader(opts) // no validation error }) test('uploader respects maxFileSize with unknown size', async () => { @@ -178,10 +255,9 @@ describe('uploader with tus protocol', () => { const uploader = new Uploader(opts) const uploadToken = uploader.token - expect(uploader.hasError()).toBe(false) // validate that the test is resolved on socket connection - uploader.awaitReady().then(uploader.uploadStream(stream)) + uploader.awaitReady().then(uploader.tryUploadStream(stream)) socketClient.connect(uploadToken) return new Promise((resolve, reject) => { diff --git a/packages/@uppy/companion/test/mockserver.js b/packages/@uppy/companion/test/mockserver.js index 5a33291d86..2261356195 100644 --- a/packages/@uppy/companion/test/mockserver.js +++ b/packages/@uppy/companion/test/mockserver.js @@ -2,13 +2,53 @@ const express = require('express') const session = require('express-session') -module.exports.getServer = (env) => { - if (env) { - Object.keys(env).forEach((key) => { - process.env[key] = env[key] - }) +const defaultEnv = { + NODE_ENV: 'test', + COMPANION_PORT: 3020, + COMPANION_DOMAIN: 'localhost:3020', + COMPANION_SELF_ENDPOINT: 'localhost:3020', + COMPANION_HIDE_METRICS: 'false', + COMPANION_HIDE_WELCOME: 'false', + + COMPANION_STREAMING_UPLOAD: 'true', + + COMPANION_PROTOCOL: 'http', + COMPANION_DATADIR: './test/output', + COMPANION_SECRET: 'secret', + + COMPANION_DROPBOX_KEY: 'dropbox_key', + COMPANION_DROPBOX_SECRET: 'dropbox_secret', + + COMPANION_BOX_KEY: 'box_key', + COMPANION_BOX_SECRET: 'box_secret', + + COMPANION_GOOGLE_KEY: 'google_key', + COMPANION_GOOGLE_SECRET: 'google_secret', + + COMPANION_INSTAGRAM_KEY: 'instagram_key', + COMPANION_INSTAGRAM_SECRET: 'instagram_secret', + + COMPANION_ZOOM_KEY: 'zoom_key', + COMPANION_ZOOM_SECRET: 'zoom_secret', + COMPANION_ZOOM_VERIFICATION_TOKEN: 'zoom_verfication_token', +} + +function updateEnv (env) { + Object.keys(env).forEach((key) => { + process.env[key] = env[key] + }) +} + +module.exports.setDefaultEnv = () => updateEnv(defaultEnv) + +module.exports.getServer = (extraEnv) => { + const env = { + ...defaultEnv, + ...extraEnv, } + updateEnv(env) + // delete from cache to force the server to reload companionOptions from the new env vars jest.resetModules() const standalone = require('../src/standalone') diff --git a/website/src/docs/companion.md b/website/src/docs/companion.md index ac635131cb..7990dba4f6 100644 --- a/website/src/docs/companion.md +++ b/website/src/docs/companion.md @@ -359,7 +359,6 @@ const options = { 18. **periodicPingStaticPayload(optional)** - A `JSON.stringify`-able JavaScript Object that will be sent as part of the JSON body in the period ping requests. - ### Provider Redirect URIs When generating your provider API keys on their corresponding developer platforms (e.g [Google Developer Console](https://console.developers.google.com/)), you’d need to provide a `redirect URI` for the OAuth authorization process. In general the redirect URI for each provider takes the format: