Fix companion dns and allow redirects from http->https again (#4895)

* fix companion dns validation

didn't work with ipv6
also improve error message for debuggability

* allow redirects from http->https again

and vice versa
because we are no longer using `request`
closes #3145

* fix lint

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

@@ -25,7 +25,7 @@ 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 protectedGot = getProtectedGot({ blockLocalIPs })
     const stream = protectedGot.stream.get(url, { responseType: 'json' })
     await prepareStream(stream)
     return stream

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

@@ -1,7 +1,6 @@
 // eslint-disable-next-line max-classes-per-file
 const http = require('node:http')
 const https = require('node:https')
-const { URL } = require('node:url')
 const dns = require('node:dns')
 const ipaddr = require('ipaddr.js')
 const got = require('got').default
@@ -9,10 +8,7 @@ const path = require('node:path')
 const contentDisposition = require('content-disposition')
 const validator = require('validator')
 
-const logger = require('../logger')
-
 const FORBIDDEN_IP_ADDRESS = 'Forbidden IP address'
-const FORBIDDEN_RESOLVED_IP_ADDRESS = 'Forbidden resolved IP address'
 
 // Example scary IPs that should return false (ipv6-to-ipv4 mapped):
 // ::FFFF:127.0.0.1
@@ -20,31 +16,6 @@ const FORBIDDEN_RESOLVED_IP_ADDRESS = 'Forbidden resolved IP address'
 const isDisallowedIP = (ipAddress) => ipaddr.parse(ipAddress).range() !== 'unicast'
 
 module.exports.FORBIDDEN_IP_ADDRESS = FORBIDDEN_IP_ADDRESS
-module.exports.FORBIDDEN_RESOLVED_IP_ADDRESS = FORBIDDEN_RESOLVED_IP_ADDRESS
-
-module.exports.getRedirectEvaluator = (rawRequestURL, isEnabled) => {
-  const requestURL = new URL(rawRequestURL)
-
-  return ({ headers }) => {
-    if (!isEnabled) return true
-
-    let redirectURL = null
-    try {
-      redirectURL = new URL(headers.location, requestURL)
-    } catch (err) {
-      return false
-    }
-
-    const shouldRedirect = redirectURL.protocol === requestURL.protocol
-    if (!shouldRedirect) {
-      logger.info(
-        `blocking redirect from ${requestURL} to ${redirectURL}`, 'redirect.protection',
-      )
-    }
-
-    return shouldRedirect
-  }
-}
 
 /**
  * Validates that the download URL is secure
@@ -83,14 +54,19 @@ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => {
       }
 
       const toValidate = Array.isArray(addresses) ? addresses : [{ address: addresses }]
-      for (const record of toValidate) {
-        if (blockLocalIPs && isDisallowedIP(record.address)) {
-          callback(new Error(FORBIDDEN_RESOLVED_IP_ADDRESS), addresses, maybeFamily)
-          return
-        }
+      // because dns.lookup seems to be called with option `all: true`, if we are on an ipv6 system,
+      // `addresses` could contain a list of ipv4 addresses as well as ipv6 mapped addresses (rfc6052) which we cannot allow
+      // however we should still allow any valid ipv4 addresses, so we filter out the invalid addresses
+      const validAddresses = !blockLocalIPs ? toValidate : toValidate.filter(({ address }) => !isDisallowedIP(address))
+
+      // and check if there's anything left after we filtered:
+      if (validAddresses.length === 0) {
+        callback(new Error(`Forbidden resolved IP address ${hostname} -> ${toValidate.map(({ address }) => address).join(', ')}`), addresses, maybeFamily)
+        return
       }
 
-      callback(err, addresses, maybeFamily)
+      const ret = Array.isArray(addresses) ? validAddresses : validAddresses[0].address;
+      callback(err, ret, maybeFamily)
     })
   }
 
@@ -108,23 +84,15 @@ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => {
 
 module.exports.getProtectedHttpAgent = getProtectedHttpAgent
 
-function getProtectedGot ({ url, blockLocalIPs }) {
+function getProtectedGot ({ blockLocalIPs }) {
   const HttpAgent = getProtectedHttpAgent({ protocol: 'http', blockLocalIPs })
   const HttpsAgent = getProtectedHttpAgent({ protocol: 'https', blockLocalIPs })
   const httpAgent = new HttpAgent()
   const httpsAgent = new HttpsAgent()
 
-  const redirectEvaluator = module.exports.getRedirectEvaluator(url, blockLocalIPs)
-
-  const beforeRedirect = (options, response) => {
-    const allowRedirect = redirectEvaluator(response)
-    if (!allowRedirect) {
-      throw new Error(`Redirect evaluator does not allow the redirect to ${response.headers.location}`)
-    }
-  }
 
   // @ts-ignore
-  return got.extend({ hooks: { beforeRedirect: [beforeRedirect] }, agent: { http: httpAgent, https: httpsAgent } })
+  return got.extend({ agent: { http: httpAgent, https: httpsAgent } })
 }
 
 module.exports.getProtectedGot = getProtectedGot
@@ -138,7 +106,7 @@ module.exports.getProtectedGot = getProtectedGot
  */
 exports.getURLMeta = async (url, blockLocalIPs = false) => {
   async function requestWithMethod (method) {
-    const protectedGot = getProtectedGot({ url, blockLocalIPs })
+    const protectedGot = getProtectedGot({ blockLocalIPs })
     const stream = protectedGot.stream(url, { method, throwHttpErrors: false })
 
     return new Promise((resolve, reject) => (

packages/@uppy/companion/test/__tests__/http-agent.js

@@ -1,37 +1,7 @@
 const nock = require('nock')
-const { getRedirectEvaluator, FORBIDDEN_IP_ADDRESS, FORBIDDEN_RESOLVED_IP_ADDRESS } = require('../../src/server/helpers/request')
+const { FORBIDDEN_IP_ADDRESS } = require('../../src/server/helpers/request')
 const { getProtectedGot } = require('../../src/server/helpers/request')
 
-describe('test getRedirectEvaluator', () => {
-  const httpURL = 'http://uppy.io'
-  const httpsURL = 'https://uppy.io'
-  const httpRedirectResp = {
-    headers: {
-      location: 'http://transloadit.com',
-    },
-  }
-
-  const httpsRedirectResp = {
-    headers: {
-      location: 'https://transloadit.com',
-    },
-  }
-
-  test('when original URL has "https:" as protocol', (done) => {
-    const shouldRedirectHttps = getRedirectEvaluator(httpsURL, true)
-    expect(shouldRedirectHttps(httpsRedirectResp)).toEqual(true)
-    expect(shouldRedirectHttps(httpRedirectResp)).toEqual(false)
-    done()
-  })
-
-  test('when original URL has "http:" as protocol', (done) => {
-    const shouldRedirectHttp = getRedirectEvaluator(httpURL, true)
-    expect(shouldRedirectHttp(httpRedirectResp)).toEqual(true)
-    expect(shouldRedirectHttp(httpsRedirectResp)).toEqual(false)
-    done()
-  })
-})
-
 afterAll(() => {
   nock.cleanAll()
   nock.restore()
@@ -41,24 +11,24 @@ describe('test protected request Agent', () => {
   test('allows URLs without IP addresses', async () => {
     nock('https://transloadit.com').get('/').reply(200)
     const url = 'https://transloadit.com'
-    await getProtectedGot({ url, blockLocalIPs: true }).get(url)
+    await getProtectedGot({ blockLocalIPs: true }).get(url)
   })
 
   test('blocks url that resolves to forbidden IP', async () => {
     const url = 'https://localhost'
-    const promise = getProtectedGot({ url, blockLocalIPs: true }).get(url)
-    await expect(promise).rejects.toThrow(new Error(FORBIDDEN_RESOLVED_IP_ADDRESS))
+    const promise = getProtectedGot({ blockLocalIPs: true }).get(url)
+    await expect(promise).rejects.toThrow(/^Forbidden resolved IP address/)
   })
 
   test('blocks private http IP address', async () => {
     const url = 'http://172.20.10.4:8090'
-    const promise = getProtectedGot({ url, blockLocalIPs: true }).get(url)
+    const promise = getProtectedGot({ blockLocalIPs: true }).get(url)
     await expect(promise).rejects.toThrow(new Error(FORBIDDEN_IP_ADDRESS))
   })
 
   test('blocks private https IP address', async () => {
     const url = 'https://172.20.10.4:8090'
-    const promise = getProtectedGot({ url, blockLocalIPs: true }).get(url)
+    const promise = getProtectedGot({ blockLocalIPs: true }).get(url)
     await expect(promise).rejects.toThrow(new Error(FORBIDDEN_IP_ADDRESS))
   })
 
@@ -87,12 +57,12 @@ describe('test protected request Agent', () => {
 
     for (const ip of ipv4s) {
       const url = `http://${ip}:8090`
-      const promise = getProtectedGot({ url, blockLocalIPs: true }).get(url)
+      const promise = getProtectedGot({ blockLocalIPs: true }).get(url)
       await expect(promise).rejects.toThrow(new Error(FORBIDDEN_IP_ADDRESS))
     }
     for (const ip of ipv6s) {
       const url = `http://[${ip}]:8090`
-      const promise = getProtectedGot({ url, blockLocalIPs: true }).get(url)
+      const promise = getProtectedGot({ blockLocalIPs: true }).get(url)
       await expect(promise).rejects.toThrow(new Error(FORBIDDEN_IP_ADDRESS))
     }
   })