@uppy/companion: unify http error responses (#5595)

* improve comments

* unify http error responses

when proxying requests
this in order to make it easier to debug when setting up companion/transloadit integration, and lessen support burden on us.
remove outdated `err.status` checks. this was added [7+ years ago](https://github.com/transloadit/uppy/blame/cf18689c1055055fc73a33fb9fe18e1046dfc8e4/packages/%40uppy/companion/src/standalone/index.js#L143) and we now use `got` which doesn't provide err.status
Instead, for any other unhandled proxied HTTP request error responses, be nice and forward the JSON response to the client for easier debugging

* Update packages/@uppy/companion/src/server/provider/error.js

examples/custom-provider/server/index.cjs

@@ -71,7 +71,7 @@ app.use((req, res) => {
 // handle server errors
 app.use((err, req, res) => {
   console.error('\x1b[31m', err.stack, '\x1b[0m')
-  res.status(err.status || 500).json({ message: err.message, error: err })
+  res.status(500).json({ message: err.message, error: err })
 })
 
 uppy.socket(app.listen(3020), uppyOptions)

examples/uppy-with-companion/server/index.js

@@ -64,7 +64,7 @@ app.use((req, res) => {
 // handle server errors
 app.use((err, req, res) => {
   console.error('\x1b[31m', err.stack, '\x1b[0m')
-  res.status(err.status || 500).json({ message: err.message, error: err })
+  res.status(500).json({ message: err.message, error: err })
 })
 
 companion.socket(app.listen(3020))

packages/@uppy/companion/src/server/controllers/get.js

@@ -1,5 +1,7 @@
 const logger = require('../logger')
 const { startDownUpload } = require('../helpers/upload')
+const { respondWithError } = require('../provider/error')
+
 
 async function get (req, res) {
   const { id } = req.params
@@ -17,6 +19,7 @@ async function get (req, res) {
     await startDownUpload({ req, res, getSize, download })
   } catch (err) {
     logger.error(err, 'controller.get.error', req.id)
+    if (respondWithError(err, res)) return
     res.status(500).json({ message: 'Failed to download file' })
   }
 }

packages/@uppy/companion/src/server/controllers/googlePicker.js

@@ -7,6 +7,7 @@ const { getURLMeta } = require('../helpers/request')
 const logger = require('../logger')
 const { downloadURL } = require('../download')
 const { getGoogleFileSize, streamGoogleFile } = require('../provider/google/drive');
+const { respondWithError } = require('../provider/error')
 
 
 const getAuthHeader = (token) => ({ authorization: `Bearer ${token}` });
@@ -49,7 +50,8 @@ const get = async (req, res) => {
     await startDownUpload({ req, res, getSize, download })
   } catch (err) {
     logger.error(err, 'controller.googlePicker.error', req.id)
-    res.status(err.status || 500).json({ message: 'failed to fetch Google Picker URL' })
+    if (respondWithError(err, res)) return
+    res.status(500).json({ message: 'failed to fetch Google Picker URL' })
   }
 }
 

packages/@uppy/companion/src/server/controllers/url.js

@@ -5,6 +5,7 @@ const { downloadURL } = require('../download')
 const { validateURL } = require('../helpers/request')
 const { getURLMeta } = require('../helpers/request')
 const logger = require('../logger')
+const { respondWithError } = require('../provider/error')
 
 /**
  * @callback downloadCallback
@@ -24,14 +25,16 @@ const meta = async (req, res) => {
     const { allowLocalUrls } = req.companion.options
     if (!validateURL(req.body.url, allowLocalUrls)) {
       logger.debug('Invalid request body detected. Exiting url meta handler.', null, req.id)
-      return res.status(400).json({ error: 'Invalid request body' })
+      res.status(400).json({ error: 'Invalid request body' })
+      return
     }
 
     const urlMeta = await getURLMeta(req.body.url, allowLocalUrls)
-    return res.json(urlMeta)
+    res.json(urlMeta)
   } catch (err) {
     logger.error(err, 'controller.url.meta.error', req.id)
-    return res.status(err.status || 500).json({ message: 'failed to fetch URL metadata' })
+    if (respondWithError(err, res)) return
+    res.status(500).json({ message: 'failed to fetch URL metadata' })
   }
 }
 
@@ -62,7 +65,8 @@ const get = async (req, res) => {
     await startDownUpload({ req, res, getSize, download })
   } catch (err) {
     logger.error(err, 'controller.url.error', req.id)
-    res.status(err.status || 500).json({ message: 'failed to fetch URL' })
+    if (respondWithError(err, res)) return
+    res.status(500).json({ message: 'failed to fetch URL' })
   }
 }
 

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

@@ -1,51 +1,38 @@
 const Uploader = require('../Uploader')
 const logger = require('../logger')
-const { respondWithError } = require('../provider/error')
 
 async function startDownUpload({ req, res, getSize, download }) {
-  try {
-    logger.debug('Starting download stream.', null, req.id)
-    const { stream, size: maybeSize } = await download()
+  logger.debug('Starting download stream.', null, req.id)
+  const { stream, size: maybeSize } = await download()
 
-    let size
-    // if the provider already knows the size, we can use that
-    if (typeof maybeSize === 'number' && !Number.isNaN(maybeSize) && maybeSize > 0) {
-      size = maybeSize
-    }
-    // if not we need to get the size
-    if (size == null) {
-      size = await getSize()
-    }
-    const { clientSocketConnectTimeout } = req.companion.options
-
-    logger.debug('Instantiating uploader.', null, req.id)
-    const uploader = new Uploader(Uploader.reqToOptions(req, size))
-
-      // "Forking" off the upload operation to background, so we can return the http request:
-      ; (async () => {
-        // wait till the client has connected to the socket, before starting
-        // the download, so that the client can receive all download/upload progress.
-        logger.debug('Waiting for socket connection before beginning remote download/upload.', null, req.id)
-        await uploader.awaitReady(clientSocketConnectTimeout)
-        logger.debug('Socket connection received. Starting remote download/upload.', null, req.id)
-
-        await uploader.tryUploadStream(stream, req)
-      })().catch((err) => logger.error(err))
-
-    // Respond the request
-    // NOTE: the Uploader will continue running after the http request is responded
-    res.status(200).json({ token: uploader.token })
-  } catch (err) {
-    if (err.name === 'ValidationError') {
-      logger.debug(err.message, 'uploader.validator.fail')
-      res.status(400).json({ message: err.message })
-      return
-    }
-
-    if (respondWithError(err, res)) return
-
-    throw err
+  let size
+  // if the provider already knows the size, we can use that
+  if (typeof maybeSize === 'number' && !Number.isNaN(maybeSize) && maybeSize > 0) {
+    size = maybeSize
   }
+  // if not we need to get the size
+  if (size == null) {
+    size = await getSize()
+  }
+  const { clientSocketConnectTimeout } = req.companion.options
+
+  logger.debug('Instantiating uploader.', null, req.id)
+  const uploader = new Uploader(Uploader.reqToOptions(req, size))
+
+    // "Forking" off the upload operation to background, so we can return the http request:
+    ; (async () => {
+      // wait till the client has connected to the socket, before starting
+      // the download, so that the client can receive all download/upload progress.
+      logger.debug('Waiting for socket connection before beginning remote download/upload.', null, req.id)
+      await uploader.awaitReady(clientSocketConnectTimeout)
+      logger.debug('Socket connection received. Starting remote download/upload.', null, req.id)
+
+      await uploader.tryUploadStream(stream, req)
+    })().catch((err) => logger.error(err))
+
+  // Respond the request
+  // NOTE: the Uploader will continue running after the http request is responded
+  res.status(200).json({ token: uploader.token })
 }
 
 module.exports = { startDownUpload }

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

@@ -148,6 +148,9 @@ module.exports.decrypt = (encrypted, secret) => {
 
 module.exports.defaultGetKey = ({ filename }) => `${crypto.randomUUID()}-${filename}`
 
+/**
+ * Our own HttpError in cases where we can't use `got`'s `HTTPError`
+ */
 class HttpError extends Error {
   statusCode
 
@@ -176,7 +179,7 @@ module.exports.prepareStream = async (stream) => new Promise((resolve, reject) =
     })
     .on('error', (err) => {
       // In this case the error object is not a normal GOT HTTPError where json is already parsed,
-      // we create our own HttpError error for this case
+      // we use our own HttpError error for this scenario.
       if (typeof err.response?.body === 'string' && typeof err.response?.statusCode === 'number') {
         let responseJson
         try {

packages/@uppy/companion/src/server/provider/error.js

@@ -30,6 +30,8 @@ class ProviderUserError extends ProviderApiError {
 /**
  * AuthError is error returned when an adapter encounters
  * an authorization error while communication with its corresponding provider
+ * this signals to the client that the access token is invalid and needs to be
+ * refreshed or the user needs to re-authenticate
  */
 class ProviderAuthError extends ProviderApiError {
   constructor() {
@@ -39,10 +41,27 @@ class ProviderAuthError extends ProviderApiError {
   }
 }
 
+function parseHttpError(err) {
+  if (err?.name === 'HTTPError') {
+    return {
+      statusCode: err.response?.statusCode,
+      body: err.response?.body,
+    }
+  }
+  if (err?.name === 'HttpError') {
+    return {
+      statusCode: err.statusCode,
+      body: err.responseJson,
+    }
+  }
+  return undefined
+}
+
 /**
  * Convert an error instance to an http response if possible
  *
  * @param {Error | ProviderApiError} err the error instance to convert to an http json response
+ * @returns {object | undefined} an object with a code and json field if the error can be converted to a response
  */
 function errorToResponse(err) {
   // @ts-ignore
@@ -50,6 +69,10 @@ function errorToResponse(err) {
     return { code: 401, json: { message: err.message } }
   }
 
+  if (err?.name === 'ValidationError') {
+    return { code: 400, json: { message: err.message } }
+  }
+
   if (err?.name === 'ProviderUserError') {
     // @ts-ignore
     return { code: 400, json: err.json }
@@ -74,6 +97,12 @@ function errorToResponse(err) {
     }
   }
 
+  const httpError = parseHttpError(err)
+  if (httpError) {
+    // We proxy the response purely for ease of debugging
+    return { code: 500, json: { statusCode: httpError.statusCode, body: httpError.body } }
+  }
+
   return undefined
 }
 
@@ -86,4 +115,4 @@ function respondWithError(err, res) {
   return false
 }
 
-module.exports = { ProviderAuthError, ProviderApiError, ProviderUserError, respondWithError }
+module.exports = { ProviderAuthError, ProviderApiError, ProviderUserError, respondWithError, parseHttpError }

packages/@uppy/companion/src/server/provider/providerErrors.js

@@ -1,5 +1,5 @@
 const logger = require('../logger')
-const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./error')
+const { ProviderApiError, ProviderUserError, ProviderAuthError, parseHttpError } = require('./error')
 
 /**
  * 
@@ -37,18 +37,11 @@ async function withProviderErrorHandling({
   try {
     return await fn()
   } catch (err) {
-    let statusCode
-    let body
+    const httpError = parseHttpError(err)
 
-    if (err?.name === 'HTTPError') {
-      statusCode = err.response?.statusCode
-      body = err.response?.body
-    } else if (err?.name === 'HttpError') {
-      statusCode = err.statusCode
-      body = err.responseJson
-    }
-
-    if (statusCode != null) {
+    // Wrap all HTTP errors according to the provider's desired error handling
+    if (httpError) {
+      const { statusCode, body } = httpError
       let knownErr
       if (isAuthError({ statusCode, body })) {
         knownErr = new ProviderAuthError()
@@ -62,8 +55,8 @@ async function withProviderErrorHandling({
       throw knownErr
     }
 
+    // non HTTP errors will be passed through
     logger.error(err, tag)
-
     throw err
   }
 }
@@ -81,4 +74,4 @@ async function withGoogleErrorHandling (providerName, tag, fn) {
   })
 }
 
-module.exports = { withProviderErrorHandling, withGoogleErrorHandling }
+module.exports = { withProviderErrorHandling, withGoogleErrorHandling, parseHttpError }

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

@@ -182,10 +182,10 @@ module.exports = function server(inputCompanionOptions) {
       } else {
         logger.error(err, 'root.error', req.id)
       }
-      res.status(err.status || 500).json({ message: 'Something went wrong', requestId: req.id })
+      res.status(500).json({ message: 'Something went wrong', requestId: req.id })
     } else {
       logger.error(err, 'root.error', req.id)
-      res.status(err.status || 500).json({ message: err.message, error: err, requestId: req.id })
+      res.status(500).json({ message: err.message, error: err, requestId: req.id })
     }
   })