From 9fb7c52cf2443e823f31327c4c6137d7e37bd13b Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 15:22:40 +0200 Subject: [PATCH 1/4] companion: pass query params to more provider methods --- .../companion/src/server/controllers/list.js | 5 +++++ .../companion/src/server/controllers/logout.js | 15 +++++++-------- .../companion/src/server/controllers/thumbnail.js | 12 ++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/list.js b/packages/@uppy/companion/src/server/controllers/list.js index 284660c1be..481bdfc8d8 100644 --- a/packages/@uppy/companion/src/server/controllers/list.js +++ b/packages/@uppy/companion/src/server/controllers/list.js @@ -1,5 +1,10 @@ const { respondWithError } = require('../provider/error') +/** + * + * @param {{ query: object, params: object, companion: object }} req + * @param {object} res + */ async function list ({ query, params, companion }, res, next) { const { accessToken } = companion.providerTokens diff --git a/packages/@uppy/companion/src/server/controllers/logout.js b/packages/@uppy/companion/src/server/controllers/logout.js index 1ec87e303f..2aebb9bee6 100644 --- a/packages/@uppy/companion/src/server/controllers/logout.js +++ b/packages/@uppy/companion/src/server/controllers/logout.js @@ -3,18 +3,17 @@ const { respondWithError } = require('../provider/error') /** * - * @param {object} req + * @param {{ query: object, params: object, companion: object, session: object }} req * @param {object} res */ -async function logout (req, res, next) { +async function logout ({ query, params, companion, session }, res, next) { const cleanSession = () => { - if (req.session.grant) { - req.session.grant.state = null - req.session.grant.dynamic = null + if (session.grant) { + session.grant.state = null + session.grant.dynamic = null } } - const { providerName } = req.params - const { companion } = req + const { providerName } = params const tokens = companion.allProvidersTokens ? companion.allProvidersTokens[providerName] : null if (!tokens) { @@ -25,7 +24,7 @@ async function logout (req, res, next) { try { const { accessToken } = tokens - const data = await companion.provider.logout({ token: accessToken, companion }) + const data = await companion.provider.logout({ companion, token: accessToken, query }) delete companion.allProvidersTokens[providerName] tokenService.removeFromCookies(res, companion.options, companion.provider.authProvider) cleanSession() diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index 0994964ed2..24e9755496 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -1,15 +1,15 @@ /** * - * @param {object} req + * @param {{ params: object, companion: object, query: object }} req * @param {object} res */ -async function thumbnail (req, res, next) { - const { providerName, id } = req.params - const { accessToken } = req.companion.allProvidersTokens[providerName] - const { provider } = req.companion +async function thumbnail ({ params, companion, query }, res, next) { + const { providerName, id } = params + const { accessToken } = companion.allProvidersTokens[providerName] + const { provider } = companion try { - const { stream } = await provider.thumbnail({ id, token: accessToken }) + const { stream } = await provider.thumbnail({ id, token: accessToken, query }) stream.pipe(res) } catch (err) { if (err.isAuthError) res.sendStatus(401) From c7a7d46c578119a683a07ed2d3ec9937a91c5e7c Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 15:33:34 +0200 Subject: [PATCH 2/4] Fix typos in comments --- packages/@uppy/companion/src/server/controllers/url.js | 2 +- packages/@uppy/companion/src/server/provider/credentials.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/url.js b/packages/@uppy/companion/src/server/controllers/url.js index 564103a0c8..b829112bc4 100644 --- a/packages/@uppy/companion/src/server/controllers/url.js +++ b/packages/@uppy/companion/src/server/controllers/url.js @@ -57,7 +57,7 @@ const downloadURL = async (url, blockLocalIPs, traceId) => { } /** - * Fteches the size and content type of a URL + * Fetches the size and content type of a URL * * @param {object} req expressJS request object * @param {object} res expressJS response object diff --git a/packages/@uppy/companion/src/server/provider/credentials.js b/packages/@uppy/companion/src/server/provider/credentials.js index fa41dbaef1..da99ffe5ef 100644 --- a/packages/@uppy/companion/src/server/provider/credentials.js +++ b/packages/@uppy/companion/src/server/provider/credentials.js @@ -27,7 +27,7 @@ async function fetchKeys (url, providerName, credentialRequestParams) { } /** - * Fetches for a providers OAuth credentials. If the config for thtat provider allows fetching + * Fetches for a providers OAuth credentials. If the config for that provider allows fetching * of the credentials via http, and the `credentialRequestParams` argument is provided, the oauth * credentials will be fetched via http. Otherwise, the credentials provided via companion options * will be used instead. From ca3b12c8c48fd09f4b7d9cb472673f9c861a665d Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 15:35:48 +0200 Subject: [PATCH 3/4] @uppy/companion-client: allow defining customQueryParams that are added to requests --- .../@uppy/companion-client/src/Provider.js | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 081f3cdc65..b04decc4f8 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -7,6 +7,11 @@ const getName = (id) => { return id.split('-').map((s) => s.charAt(0).toUpperCase() + s.slice(1)).join(' ') } +const queryString = (params, prefix = '?') => { + const str = new URLSearchParams(params).toString() + return str ? `${prefix}${str}` : '' +} + export default class Provider extends RequestClient { #refreshingTokenPromise @@ -72,20 +77,22 @@ export default class Provider extends RequestClient { } authUrl (queries = {}) { - const params = new URLSearchParams(queries) - if (this.preAuthToken) { - params.set('uppyPreAuthToken', this.preAuthToken) - } - - return `${this.hostname}/${this.id}/connect?${params}` + const qs = queryString({ + ...queries, + ...this.cutomQueryParams, + ...(this.preAuthToken && { + uppyPreAuthToken: this.preAuthToken, + }), + }) + return `${this.hostname}/${this.id}/connect${qs}` } refreshTokenUrl () { - return `${this.hostname}/${this.id}/refresh-token` + return `${this.hostname}/${this.id}/refresh-token${queryString(this.cutomQueryParams)}` } fileUrl (id) { - return `${this.hostname}/${this.id}/get/${id}` + return `${this.hostname}/${this.id}/get/${id}${queryString(this.cutomQueryParams)}` } /** @protected */ @@ -132,11 +139,11 @@ export default class Provider extends RequestClient { } list (directory, options) { - return this.get(`${this.id}/list/${directory || ''}`, options) + return this.get(`${this.id}/list/${directory || ''}${queryString(this.cutomQueryParams)}`, options) } async logout (options) { - const response = await this.get(`${this.id}/logout`, options) + const response = await this.get(`${this.id}/logout${queryString(this.cutomQueryParams)}`, options) await this.#removeAuthToken() return response } From 968fe8a855b94f8d41adac6646cbfdcaff5b937d Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 15:45:16 +0200 Subject: [PATCH 4/4] @uppy/companion: pass dynamic parameters to grant This allows to define dynamic properties for grant providers. E.g. `[subdomain]` can be automatically replaced in `authorize_url` and `access_url`. For that to work we need to include the dynamic property in the redirect to the grant middleware. We merge the grantConfig back to the providerConfig, so we can determine the required query params based on the `dynamic` property. This avoids an attacker overriding unexpected properties (if we simply serialize and pass along `query`) --- packages/@uppy/companion/src/companion.js | 1 + .../companion/src/server/controllers/connect.js | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index 2f29436488..92e2464476 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -73,6 +73,7 @@ module.exports.app = (optionsArg = {}) => { const providers = providerManager.getDefaultProviders() providerManager.addProviderOptions(options, grantConfig) + options.providerOptions = merge(options.providerOptions, grantConfig) const { customProviders } = options if (customProviders) { diff --git a/packages/@uppy/companion/src/server/controllers/connect.js b/packages/@uppy/companion/src/server/controllers/connect.js index 1e98927744..0b1fb0ff0a 100644 --- a/packages/@uppy/companion/src/server/controllers/connect.js +++ b/packages/@uppy/companion/src/server/controllers/connect.js @@ -1,6 +1,11 @@ const atob = require('atob') const oAuthState = require('../helpers/oauth-state') +const queryString = (params, prefix = '') => { + const str = new URLSearchParams(params).toString() + return str ? `${prefix}${str}` : '' +} + /** * initializes the oAuth flow for a provider. * @@ -27,5 +32,10 @@ module.exports = function connect (req, res) { state = oAuthState.addToState(state, { preAuthToken: req.query.uppyPreAuthToken }, secret) } - res.redirect(req.companion.buildURL(`/connect/${req.companion.provider.authProvider}?state=${state}`, true)) + const providerName = req.companion.provider.authProvider + const qs = queryString(Object.fromEntries( + req.companion.options.providerOptions[providerName]?.dynamic?.map(p => [p, req.query[p]]) || [], + ), '&') + + res.redirect(req.companion.buildURL(`/connect/${providerName}?state=${state}${qs}`, true)) }