From 875bc0bbeac2705fa8ff69d50ffbddb63a7985f6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 8 Nov 2022 15:20:22 +0100 Subject: [PATCH] @uppy/companion-client: prevent preflight race condition (#4182) Co-authored-by: Antoine du Hamel --- .../@uppy/companion-client/src/Provider.js | 28 ++- .../companion-client/src/RequestClient.js | 196 +++++++++--------- 2 files changed, 106 insertions(+), 118 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 92c7a8547..b30b6b4f4 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/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 - } + 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 } - }) + 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 diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 637008bdd..25a14cd93 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/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. - preflight (path) { - if (this.preflightDone) { - return Promise.resolve(this.allowedHeaders.slice()) - } + 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 - 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()) + 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 } - this.preflightDone = true - return this.allowedHeaders.slice() - }) - .catch((err) => { + + 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') - 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 + } + })() + + allowedHeadersCache.set(this.hostname, promise) + return promise } - 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 - }) + 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 + })) } - 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)) - } - - 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 }) } }