fix unpslash author meta, sanitize metadata to strings and improve co…

…mpanion tests (#3478)

* fix broken npm run test

* improve jest / companion

add `npm run test:companion:watch`

move env variables into code to make testing easier
this allows for watching (source is causing problems with npm scripts)
also now we can run `corepack yarn test:companion -t 'single test'`

* fix root project jest

make sure we don't run companion tests on npm run test:unit (as they don't work in a browser/jsdom environment)

* improve validation logic

previously incorrect options gives an Uploader object in an incorrect state

* rewrite uploader to make it more testable

* add test for xhr

* check that metadata values are strings

* fix nested meta

causing error #3477

* convert meta to strings instead

like the official FormData spec does

* fix broken companion dev #3473

* fix botched merge

* fix botched merge and remove --fix

* fix botchedf merge

* quick fix

* .

* remove eslint fix

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": {

packages/@uppy/companion/.gitignore

@@ -31,7 +31,6 @@ config/auth.js
 
 *.pem
 env.*
-!env.test.sh
 
 output/*
 test/output/*

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"

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,91 +279,71 @@ 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
     // validation, because the destination is determined
     // 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

packages/@uppy/companion/src/server/helpers/upload.js

@@ -2,19 +2,15 @@ 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()
 
     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 })

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