@uppy/companion-client: avoid unnecessary preflight requests (#4462)

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests, we have so many preflight requests because we add `Uppy-Versions` header and `Content-Type: application/json` to each request. It's particularly not useful for for requests with no body / an empty body because the remote Companion doesn't check `Content-Type`.

It was added in b25f243627c30cce7705689f1f721175b98e0c07, and since f0f1105ef016ea743d3764cb6eb129a228529aad, it is only used for log 
purposes.

Co-authored-by: Mikael Finstad <finstaden@gmail.com>

package.json

@@ -154,7 +154,7 @@
     "test:locale-packs:unused": "yarn workspace @uppy-dev/locale-pack test unused",
     "test:locale-packs:warnings": "yarn workspace @uppy-dev/locale-pack test warnings",
     "test:type": "yarn workspaces foreach -piv --include '@uppy/*' --exclude '@uppy/{angular,react-native,locales,companion,provider-views,robodog,svelte}' exec tsd",
-    "test:unit": "yarn run build:lib && yarn test:watch",
+    "test:unit": "yarn run build:lib && yarn test:watch --run",
     "test:watch": "vitest --environment jsdom --dir packages/@uppy",
     "test": "npm-run-all lint test:locale-packs:unused test:unit test:type test:companion",
     "uploadcdn": "yarn node ./bin/upload-to-cdn.js",

packages/@uppy/companion-client/src/RequestClient.js

@@ -55,9 +55,6 @@ async function handleJSONResponse (res) {
   throw new HttpError({ statusCode: res.status, message: errMsg })
 }
 
-// todo pull out into core instead?
-const allowedHeadersCache = new Map()
-
 export default class RequestClient {
   static VERSION = packageJson.version
 
@@ -84,11 +81,13 @@ export default class RequestClient {
     return stripSlash(companion && companion[host] ? companion[host] : host)
   }
 
-  async headers () {
+  async headers (emptyBody = false) {
     const defaultHeaders = {
       Accept: 'application/json',
-      'Content-Type': 'application/json',
-      'Uppy-Versions': `@uppy/companion-client=${RequestClient.VERSION}`,
+      ...(emptyBody ? undefined : {
+        // Passing those headers on requests with no data forces browsers to first make a preflight request.
+        'Content-Type': 'application/json',
+      }),
     }
 
     return {
@@ -117,88 +116,10 @@ export default class RequestClient {
     return `${this.hostname}/${url}`
   }
 
-  /*
-    Preflight was added to avoid breaking change between older Companion-client versions and
-    newer Companion versions and vice-versa. Usually the break will manifest via CORS errors because a
-    version of companion-client could be sending certain headers to a version of Companion server that
-    does not support those headers. In which case, the default preflight would lead to CORS.
-    So to avoid those errors, we do preflight ourselves, to see what headers the Companion server
-    we are communicating with allows. And based on that, companion-client knows what headers to
-    send and what headers to not send.
-
-    The preflight only happens once throughout the life-cycle of a certain
-    Companion-client <-> Companion-server pair (allowedHeadersCache).
-    Subsequent requests use the cached result of the preflight.
-    However if there is an error retrieving the allowed headers, we will try again next time
-  */
-  async preflight (path) {
-    const allowedHeadersCached = allowedHeadersCache.get(this.hostname)
-    if (allowedHeadersCached != null) return allowedHeadersCached
-
-    const fallbackAllowedHeaders = [
-      'accept',
-      'content-type',
-      'uppy-auth-token',
-    ]
-
-    const promise = (async () => {
-      try {
-        const response = await fetch(this.#getUrl(path), { method: 'OPTIONS' })
-
-        const header = response.headers.get('access-control-allow-headers')
-        if (header == null || header === '*') {
-          allowedHeadersCache.set(this.hostname, fallbackAllowedHeaders)
-          return fallbackAllowedHeaders
-        }
-
-        this.uppy.log(
-          `[CompanionClient] adding allowed preflight headers to companion cache: ${this.hostname} ${header}`,
-        )
-
-        const allowedHeaders = header
-          .split(',')
-          .map((headerName) => headerName.trim().toLowerCase())
-        allowedHeadersCache.set(this.hostname, allowedHeaders)
-        return allowedHeaders
-      } catch (err) {
-        this.uppy.log(
-          `[CompanionClient] unable to make preflight request ${err}`,
-          'warning',
-        )
-        // If the user gets a network error or similar, we should try preflight
-        // again next time, or else we might get incorrect behaviour.
-        allowedHeadersCache.delete(this.hostname) // re-fetch next time
-        return fallbackAllowedHeaders
-      }
-    })()
-
-    allowedHeadersCache.set(this.hostname, promise)
-    return promise
-  }
-
-  async preflightAndHeaders (path) {
-    const [allowedHeaders, headers] = await Promise.all([
-      this.preflight(path),
-      this.headers(),
-    ])
-    // filter to keep only allowed Headers
-    return Object.fromEntries(
-      Object.entries(headers).filter(([header]) => {
-        if (!allowedHeaders.includes(header.toLowerCase())) {
-          this.uppy.log(
-            `[CompanionClient] excluding disallowed header ${header}`,
-          )
-          return false
-        }
-        return true
-      }),
-    )
-  }
-
   /** @protected */
   async request ({ path, method = 'GET', data, skipPostResponse, signal }) {
     try {
-      const headers = await this.preflightAndHeaders(path)
+      const headers = await this.headers(!data)
       const response = await fetchWithNetworkError(this.#getUrl(path), {
         method,
         signal,
@@ -280,7 +201,7 @@ export default class RequestClient {
               || (err.statusCode >= 500 && err.statusCode <= 599 && ![501, 505].includes(err.statusCode))
             )
             if (err instanceof HttpError && !isRetryableHttpError()) throw new AbortError(err);
-  
+
             // p-retry will retry most other errors,
             // but it will not retry TypeError (except network error TypeErrors)
             throw err
@@ -359,7 +280,7 @@ export default class RequestClient {
           socket.send(JSON.stringify({
             action,
             payload: payload ?? {},
-          }))    
+          }))
         };
 
         function sendState() {
@@ -379,7 +300,7 @@ export default class RequestClient {
             socketAbortController?.abort?.()
             reject(err)
           }
-  
+
           // todo instead implement the ability for users to cancel / retry *currently uploading files* in the UI
           function resetActivityTimeout() {
             clearTimeout(activityTimeout)
@@ -414,7 +335,7 @@ export default class RequestClient {
 
                   try {
                     const { action, payload } = JSON.parse(e.data)
-            
+
                     switch (action) {
                       case 'progress': {
                         emitSocketProgress(this, payload, file)
@@ -430,8 +351,8 @@ export default class RequestClient {
                         const { message } = payload.error
                         throw Object.assign(new Error(message), { cause: payload.error })
                       }
-                        default:
-                          this.uppy.log(`Companion socket unknown action ${action}`, 'warning')
+                      default:
+                        this.uppy.log(`Companion socket unknown action ${action}`, 'warning')
                     }
                   } catch (err) {
                     onFatalError(err)
@@ -444,7 +365,7 @@ export default class RequestClient {
                   if (socket) socket.close()
                   socket = undefined
                 }
-        
+
                 socketAbortController.signal.addEventListener('abort', () => {
                   closeSocket()
                 })

packages/@uppy/companion/package.json

@@ -63,7 +63,6 @@
     "node-schedule": "2.1.0",
     "prom-client": "14.0.1",
     "redis": "4.2.0",
-    "semver": "7.5.3",
     "serialize-error": "^2.1.0",
     "serialize-javascript": "^6.0.0",
     "tus-js-client": "^3.0.0",

packages/@uppy/companion/src/config/companion.js

@@ -18,7 +18,6 @@ const defaultOptions = {
   },
   enableUrlEndpoint: true, // todo next major make this default false
   allowLocalUrls: false,
-  logClientVersion: true,
   periodicPingUrls: [],
   streamingUpload: false,
   clientSocketConnectTimeout: 60000,

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

@@ -19,10 +19,6 @@ module.exports = function connect (req, res) {
     state = oAuthState.addToState(state, { companionInstance: req.companion.buildURL('', true) }, secret)
   }
 
-  if (req.companion.clientVersion) {
-    state = oAuthState.addToState(state, { clientVersion: req.companion.clientVersion }, secret)
-  }
-
   if (req.query.uppyPreAuthToken) {
     state = oAuthState.addToState(state, { preAuthToken: req.query.uppyPreAuthToken }, secret)
   }

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

@@ -1,15 +0,0 @@
-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) => {
-  return semver.gte(
-    semver.coerce(v1).version,
-    semver.coerce(v2).version,
-  )
-}

packages/@uppy/companion/src/server/middlewares.js

@@ -123,13 +123,13 @@ exports.cors = (options = {}) => (req, res, next) => {
   const exposeHeadersSet = new Set(existingExposeHeaders?.split(',')?.map((method) => method.trim().toLowerCase()))
 
   // exposed so it can be accessed for our custom uppy client preflight
-  exposeHeadersSet.add('access-control-allow-headers')
+  exposeHeadersSet.add('access-control-allow-headers') // todo remove in next major, see https://github.com/transloadit/uppy/pull/4462
   if (options.sendSelfEndpoint) exposeHeadersSet.add('i-am')
 
   // Needed for basic operation: https://github.com/transloadit/uppy/issues/3021
   const allowedHeaders = [
     'uppy-auth-token',
-    'uppy-versions',
+    'uppy-versions', // todo remove in the future? see https://github.com/transloadit/uppy/pull/4462
     'uppy-credentials-params',
     'authorization',
     'origin',
@@ -191,18 +191,12 @@ exports.getCompanionMiddleware = (options) => {
    * @param {Function} next
    */
   const middleware = (req, res, next) => {
-    const versionFromQuery = req.query.uppyVersions ? decodeURIComponent(req.query.uppyVersions) : null
     req.companion = {
       options,
       s3Client: getS3Client(options),
       authToken: req.header('uppy-auth-token') || req.query.uppyAuthToken,
-      clientVersion: req.header('uppy-versions') || versionFromQuery || '1.0.0',
       buildURL: getURLBuilder(options),
     }
-
-    if (options.logClientVersion) {
-      logger.info(`uppy client version ${req.companion.clientVersion}`, 'companion.client.version')
-    }
     next()
   }
 

packages/@uppy/companion/test/__tests__/callback.js

@@ -43,7 +43,7 @@ describe('test authentication callback', () => {
   test('the token gets sent via html', () => {
     // see mock ../../src/server/helpers/oauth-state above for state values
     return request(authServer)
-      .get(`/dropbox/send-token?uppyAuthToken=${token}&state=state-with-newer-version`)
+      .get(`/dropbox/send-token?uppyAuthToken=${token}`)
       .expect(200)
       .expect((res) => {
         const body = `

packages/@uppy/companion/test/mockoauthstate.js

@@ -2,23 +2,11 @@ module.exports = () => {
   return {
     generateState: () => 'some-cool-nice-encrytpion',
     addToState: () => 'some-cool-nice-encrytpion',
-    getFromState: (state, key) => {
+    getFromState: (state) => {
       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'
     },
   }

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

@@ -242,9 +242,8 @@ export default class ProviderView extends View {
   }
 
   async handleAuth () {
-    const clientVersion = `@uppy/provider-views=${ProviderView.VERSION}`
     try {
-      await this.provider.login({ uppyVersions: clientVersion })
+      await this.provider.login()
       this.plugin.setPluginState({ authenticated: true })
       this.preFirstRender()
     } catch (e) {

yarn.lock

@@ -9356,7 +9356,6 @@ __metadata:
     node-schedule: 2.1.0
     prom-client: 14.0.1
     redis: 4.2.0
-    semver: 7.5.3
     serialize-error: ^2.1.0
     serialize-javascript: ^6.0.0
     supertest: 6.2.4