From 5e208bd2ba7ab879c322a644df24931a15b9e12e Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 16:02:20 +0200 Subject: [PATCH 1/5] @uppy/companion: make validateURL reusable --- .../companion/src/server/controllers/url.js | 25 +---------------- .../companion/src/server/helpers/request.js | 28 ++++++++++++++++++- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/url.js b/packages/@uppy/companion/src/server/controllers/url.js index 564103a0c8..8d8fcc37cd 100644 --- a/packages/@uppy/companion/src/server/controllers/url.js +++ b/packages/@uppy/companion/src/server/controllers/url.js @@ -1,34 +1,11 @@ const express = require('express') -const validator = require('validator') const { startDownUpload } = require('../helpers/upload') const { prepareStream } = require('../helpers/utils') +const { validateURL } = require('../helpers/request') const { getURLMeta, getProtectedGot } = require('../helpers/request') const logger = require('../logger') -/** - * Validates that the download URL is secure - * - * @param {string} url the url to validate - * @param {boolean} ignoreTld whether to allow local addresses - */ -const validateURL = (url, ignoreTld) => { - if (!url) { - return false - } - - const validURLOpts = { - protocols: ['http', 'https'], - require_protocol: true, - require_tld: !ignoreTld, - } - if (!validator.isURL(url, validURLOpts)) { - return false - } - - return true -} - /** * @callback downloadCallback * @param {Error} err diff --git a/packages/@uppy/companion/src/server/helpers/request.js b/packages/@uppy/companion/src/server/helpers/request.js index 0fe52cefca..e07a040863 100644 --- a/packages/@uppy/companion/src/server/helpers/request.js +++ b/packages/@uppy/companion/src/server/helpers/request.js @@ -7,6 +7,7 @@ const ipaddr = require('ipaddr.js') const got = require('got').default const path = require('node:path') const contentDisposition = require('content-disposition') +const validator = require('validator') const logger = require('../logger') @@ -46,7 +47,32 @@ module.exports.getRedirectEvaluator = (rawRequestURL, isEnabled) => { } /** - * Returns http Agent that will prevent requests to private IPs (to preven SSRF) + * Validates that the download URL is secure + * + * @param {string} url the url to validate + * @param {boolean} allowLocalUrls whether to allow local addresses + */ +const validateURL = (url, allowLocalUrls) => { + if (!url) { + return false + } + + const validURLOpts = { + protocols: ['http', 'https'], + require_protocol: true, + require_tld: !allowLocalUrls, + } + if (!validator.isURL(url, validURLOpts)) { + return false + } + + return true +} + +module.exports.validateURL = validateURL + +/** + * Returns http Agent that will prevent requests to private IPs (to prevent SSRF) */ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => { function dnsLookup (hostname, options, callback) { From fd79e7ab9be20f43fd414db62c21523dec2c3f2b Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 16:03:41 +0200 Subject: [PATCH 2/5] @uppy/companion: make getProtectedHttpAgent reusable This makes it possible to create protected http agents in providers and pass them on to networking libraries. --- packages/@uppy/companion/src/server/helpers/request.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/@uppy/companion/src/server/helpers/request.js b/packages/@uppy/companion/src/server/helpers/request.js index e07a040863..74fa1bfadb 100644 --- a/packages/@uppy/companion/src/server/helpers/request.js +++ b/packages/@uppy/companion/src/server/helpers/request.js @@ -121,6 +121,8 @@ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => { return protocol.startsWith('https') ? HttpsAgent : HttpAgent } +module.exports.getProtectedHttpAgent = getProtectedHttpAgent + function getProtectedGot ({ url, blockLocalIPs }) { const HttpAgent = getProtectedHttpAgent({ protocol: 'http', blockLocalIPs }) const HttpsAgent = getProtectedHttpAgent({ protocol: 'https', blockLocalIPs }) From 1ec1d5b5d25a12242a3d4f0dbcc50d7b25ace7be Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 6 Jul 2023 16:18:46 +0200 Subject: [PATCH 3/5] @uppy/companion: make allowLocalUrls and providerOptions available to providers --- packages/@uppy/companion/src/server/provider/Provider.js | 6 ++++-- packages/@uppy/companion/src/server/provider/index.js | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index 2268b0aec4..b7f21ffd0e 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -4,11 +4,13 @@ class Provider { /** * - * @param {object} options + * @param {{providerName: string, allowLocalUrls: boolean, providerOptions: object}} options */ - constructor (options) { // eslint-disable-line no-unused-vars + constructor ({ allowLocalUrls, providerOptions }) { // Some providers might need cookie auth for the thumbnails fetched via companion this.needsCookieAuth = false + this.allowLocalUrls = allowLocalUrls + this.providerOptions = providerOptions return this } diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index 0e06f5f459..df6c0b9f47 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -53,7 +53,9 @@ module.exports.getProviderMiddleware = (providers) => { const middleware = (req, res, next, providerName) => { const ProviderClass = providers[providerName] if (ProviderClass && validOptions(req.companion.options)) { - req.companion.provider = new ProviderClass({ providerName }) + const providerOptions = req.companion.options.providerOptions[providerName] || {} + const { allowLocalUrls } = req.companion.options + req.companion.provider = new ProviderClass({ providerName, providerOptions, allowLocalUrls }) req.companion.providerClass = ProviderClass if (isOAuthProvider(ProviderClass.authProvider)) { From 9e101569c81ca7551da55cde5d2c99d1ae3e6ae1 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Fri, 21 Jul 2023 08:57:59 +0200 Subject: [PATCH 4/5] Do not pass in providerOptions to providers for now ... will send in a separate PR. --- packages/@uppy/companion/src/server/provider/Provider.js | 5 ++--- packages/@uppy/companion/src/server/provider/index.js | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index b7f21ffd0e..074d18d156 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -4,13 +4,12 @@ class Provider { /** * - * @param {{providerName: string, allowLocalUrls: boolean, providerOptions: object}} options + * @param {{providerName: string, allowLocalUrls: boolean}} options */ - constructor ({ allowLocalUrls, providerOptions }) { + constructor ({ allowLocalUrls }) { // Some providers might need cookie auth for the thumbnails fetched via companion this.needsCookieAuth = false this.allowLocalUrls = allowLocalUrls - this.providerOptions = providerOptions return this } diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index df6c0b9f47..e49aff660d 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -53,9 +53,8 @@ module.exports.getProviderMiddleware = (providers) => { const middleware = (req, res, next, providerName) => { const ProviderClass = providers[providerName] if (ProviderClass && validOptions(req.companion.options)) { - const providerOptions = req.companion.options.providerOptions[providerName] || {} const { allowLocalUrls } = req.companion.options - req.companion.provider = new ProviderClass({ providerName, providerOptions, allowLocalUrls }) + req.companion.provider = new ProviderClass({ providerName, allowLocalUrls }) req.companion.providerClass = ProviderClass if (isOAuthProvider(ProviderClass.authProvider)) { From 7bc620a2b507ceba6022b7eba1817a668c0a0629 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 7 Aug 2023 00:00:59 +0200 Subject: [PATCH 5/5] add todo about naming inconsistency --- packages/@uppy/companion/src/server/controllers/url.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/@uppy/companion/src/server/controllers/url.js b/packages/@uppy/companion/src/server/controllers/url.js index 8d8fcc37cd..616c6368b9 100644 --- a/packages/@uppy/companion/src/server/controllers/url.js +++ b/packages/@uppy/companion/src/server/controllers/url.js @@ -22,6 +22,8 @@ const logger = require('../logger') * @returns {Promise} */ const downloadURL = async (url, blockLocalIPs, traceId) => { + // TODO in next major, rename all blockLocalIPs to allowLocalUrls and invert the bool, to make it consistent + // see discussion https://github.com/transloadit/uppy/pull/4554/files#r1268677162 try { const protectedGot = getProtectedGot({ url, blockLocalIPs }) const stream = protectedGot.stream.get(url, { responseType: 'json' })