@uppy/companion-client: prevent preflight race condition (#4182)

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

@@ -19,25 +19,23 @@ export default class Provider extends RequestClient {
     this.preAuthToken = null
   }
 
-  headers () {
-    return Promise.all([super.headers(), this.getAuthToken()])
-      .then(([headers, token]) => {
-        const authHeaders = {}
-        if (token) {
-          authHeaders['uppy-auth-token'] = token
-        }
-
-        if (this.companionKeysParams) {
-          authHeaders['uppy-credentials-params'] = btoa(
-            JSON.stringify({ params: this.companionKeysParams }),
-          )
-        }
-        return { ...headers, ...authHeaders }
-      })
+  async headers () {
+    const [headers, token] = await Promise.all([super.headers(), this.getAuthToken()])
+    const authHeaders = {}
+    if (token) {
+      authHeaders['uppy-auth-token'] = token
+    }
+
+    if (this.companionKeysParams) {
+      authHeaders['uppy-credentials-params'] = btoa(
+        JSON.stringify({ params: this.companionKeysParams }),
+      )
+    }
+    return { ...headers, ...authHeaders }
   }
 
   onReceiveResponse (response) {
-    response = super.onReceiveResponse(response) // eslint-disable-line no-param-reassign
+    super.onReceiveResponse(response)
     const plugin = this.uppy.getPlugin(this.pluginId)
     const oldAuthenticated = plugin.getPluginState().authenticated
     const authenticated = oldAuthenticated ? response.status !== 401 : response.status < 400

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

@@ -17,34 +17,31 @@ async function handleJSONResponse (res) {
   }
 
   const jsonPromise = res.json()
-
-  if (res.status < 200 || res.status > 300) {
-    let errMsg = `Failed request with status: ${res.status}. ${res.statusText}`
-    try {
-      const errData = await jsonPromise
-      errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg
-      errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg
-    } finally {
-      // eslint-disable-next-line no-unsafe-finally
-      throw new Error(errMsg)
-    }
+  if (res.ok) {
+    return jsonPromise
   }
-  return jsonPromise
+
+  let errMsg = `Failed request with status: ${res.status}. ${res.statusText}`
+  try {
+    const errData = await jsonPromise
+    errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg
+    errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg
+  } catch { /* if the response contains invalid JSON, let's ignore the error */ }
+  throw new Error(errMsg)
 }
 
+// todo pull out into core instead?
+const allowedHeadersCache = new Map()
+
 export default class RequestClient {
   static VERSION = packageJson.version
 
   #companionHeaders
 
-  #getPostResponseFunc = skip => response => (skip ? response : this.onReceiveResponse(response))
-
   constructor (uppy, opts) {
     this.uppy = uppy
     this.opts = opts
     this.onReceiveResponse = this.onReceiveResponse.bind(this)
-    this.allowedHeaders = ['accept', 'content-type', 'uppy-auth-token']
-    this.preflightDone = false
     this.#companionHeaders = opts?.companionHeaders
   }
 
@@ -60,31 +57,30 @@ export default class RequestClient {
     return stripSlash(companion && companion[host] ? companion[host] : host)
   }
 
-  static defaultHeaders = {
-    Accept: 'application/json',
-    'Content-Type': 'application/json',
-    'Uppy-Versions': `@uppy/companion-client=${RequestClient.VERSION}`,
-  }
+  async headers () {
+    const defaultHeaders = {
+      Accept: 'application/json',
+      'Content-Type': 'application/json',
+      'Uppy-Versions': `@uppy/companion-client=${RequestClient.VERSION}`,
+    }
 
-  headers () {
-    return Promise.resolve({
-      ...RequestClient.defaultHeaders,
+    return {
+      ...defaultHeaders,
       ...this.#companionHeaders,
-    })
+    }
   }
 
-  onReceiveResponse (response) {
+  onReceiveResponse ({ headers }) {
     const state = this.uppy.getState()
     const companion = state.companion || {}
     const host = this.opts.companionUrl
-    const { headers } = response
+
     // Store the self-identified domain name for the Companion instance we just hit.
     if (headers.has('i-am') && headers.get('i-am') !== companion[host]) {
       this.uppy.setState({
         companion: { ...companion, [host]: headers.get('i-am') },
       })
     }
-    return response
   }
 
   #getUrl (url) {
@@ -94,92 +90,86 @@ export default class RequestClient {
     return `${this.hostname}/${url}`
   }
 
-  #errorHandler (method, path) {
-    return (err) => {
-      if (!err?.isAuthError) {
-        // eslint-disable-next-line no-param-reassign
-        err = new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, { cause: err })
-      }
-      return Promise.reject(err)
-    }
-  }
+  /*
+    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) {
+          allowedHeadersCache.set(this.hostname, fallbackAllowedHeaders)
+          return fallbackAllowedHeaders
+        }
 
-  preflight (path) {
-    if (this.preflightDone) {
-      return Promise.resolve(this.allowedHeaders.slice())
-    }
+        this.uppy.log(`[CompanionClient] adding allowed preflight headers to companion cache: ${this.hostname} ${header}`)
 
-    return fetch(this.#getUrl(path), {
-      method: 'OPTIONS',
-    })
-      .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
-        return this.allowedHeaders.slice()
-      })
-      .catch((err) => {
+        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')
-        this.preflightDone = true
-        return this.allowedHeaders.slice()
-      })
-  }
+        // 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
+      }
+    })()
 
-  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.includes(header.toLowerCase())) {
-            this.uppy.log(`[CompanionClient] excluding disallowed header ${header}`)
-            delete headers[header] // eslint-disable-line no-param-reassign
-          }
-        })
-
-        return headers
-      })
+    allowedHeadersCache.set(this.hostname, promise)
+    return promise
   }
 
-  get (path, skipPostResponse) {
-    const method = 'get'
-    return this.preflightAndHeaders(path)
-      .then((headers) => fetchWithNetworkError(this.#getUrl(path), {
-        method,
-        headers,
-        credentials: this.opts.companionCookiesRule || 'same-origin',
-      }))
-      .then(this.#getPostResponseFunc(skipPostResponse))
-      .then(handleJSONResponse)
-      .catch(this.#errorHandler(method, path))
-  }
-
-  post (path, data, skipPostResponse) {
-    const method = 'post'
-    return this.preflightAndHeaders(path)
-      .then((headers) => fetchWithNetworkError(this.#getUrl(path), {
-        method,
-        headers,
-        credentials: this.opts.companionCookiesRule || 'same-origin',
-        body: JSON.stringify(data),
-      }))
-      .then(this.#getPostResponseFunc(skipPostResponse))
-      .then(handleJSONResponse)
-      .catch(this.#errorHandler(method, path))
+  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
+    }))
   }
 
-  delete (path, data, skipPostResponse) {
-    const method = 'delete'
-    return this.preflightAndHeaders(path)
-      .then((headers) => fetchWithNetworkError(`${this.hostname}/${path}`, {
+  async #request ({ path, method = 'GET', data, skipPostResponse }) {
+    try {
+      const headers = await this.preflightAndHeaders(path)
+      const response = await fetchWithNetworkError(this.#getUrl(path), {
         method,
         headers,
         credentials: this.opts.companionCookiesRule || 'same-origin',
         body: data ? JSON.stringify(data) : null,
-      }))
-      .then(this.#getPostResponseFunc(skipPostResponse))
-      .then(handleJSONResponse)
-      .catch(this.#errorHandler(method, path))
+      })
+      if (!skipPostResponse) this.onReceiveResponse(response)
+      return handleJSONResponse(response)
+    } catch (err) {
+      if (err?.isAuthError) throw err
+      throw new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, { cause: err })
+    }
   }
+
+  async get (path, skipPostResponse) { return this.#request({ path, skipPostResponse }) }
+
+  async post (path, data, skipPostResponse) { return this.#request({ path, method: 'POST', data, skipPostResponse }) }
+
+  async delete (path, data, skipPostResponse) { return this.#request({ path, method: 'DELETE', data, skipPostResponse }) }
 }