Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Feb 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c95f70f
fix companion dns validation
mifi Jan 29, 2024
e470116
allow redirects from http->https again
mifi Jan 29, 2024
07f1d7f
fix lint
mifi Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@uppy/companion/src/server/controllers/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
60 changes: 14 additions & 46 deletions packages/@uppy/companion/src/server/helpers/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,21 @@
// 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
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
// ::ffff:7f00:1
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
Expand Down Expand Up @@ -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)
})
}

Expand All @@ -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
Expand All @@ -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) => (
Expand Down
46 changes: 8 additions & 38 deletions packages/@uppy/companion/test/__tests__/http-agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
})
})

Comment on lines -5 to -34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we remove those tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are removed because the old library we used didn't support redirects to a different protocol. Now we use got which handles this internally.

afterAll(() => {
nock.cleanAll()
nock.restore()
Expand All @@ -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))
})

Expand Down Expand Up @@ -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))
}
})
Expand Down
Loading