Skip to content

Commit

Permalink
@uppy/companion: invert some internal boolean options (#5198)
Browse files Browse the repository at this point in the history
  • Loading branch information
mifi authored and aduh95 committed May 30, 2024
1 parent 3878f2e commit 1b1e5c7
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 26 deletions.
7 changes: 7 additions & 0 deletions docs/guides/migration-guides.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ These cover all the major Uppy versions and how to migrate to them.
- `access-control-allow-headers` is no longer included in
`Access-Control-Expose-Headers`, and `uppy-versions` is no longer an allowed
header. We are not aware of any issues this might cause.
- Internal refactoring (probably won’t affect you)
- `getProtectedGot` parameter `blockLocalIPs` changed to `allowLocalIPs`
(inverted boolean).
- `getURLMeta` 2nd (boolean) argument inverted.
- `getProtectedHttpAgent` parameter `blockLocalIPs` changed to `allowLocalIPs`
(inverted boolean).
- `downloadURL` 2nd (boolean) argument inverted.

### `@uppy/companion-client`

Expand Down
14 changes: 6 additions & 8 deletions packages/@uppy/companion/src/server/controllers/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@ const logger = require('../logger')
* to the callback chunk by chunk.
*
* @param {string} url
* @param {boolean} blockLocalIPs
* @param {boolean} allowLocalIPs
* @param {string} traceId
* @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
const downloadURL = async (url, allowLocalIPs, traceId) => {
try {
const protectedGot = await getProtectedGot({ blockLocalIPs })
const protectedGot = await getProtectedGot({ allowLocalIPs })
const stream = protectedGot.stream.get(url, { responseType: 'json' })
await prepareStream(stream)
return stream
Expand All @@ -50,7 +48,7 @@ const meta = async (req, res) => {
return res.status(400).json({ error: 'Invalid request body' })
}

const urlMeta = await getURLMeta(req.body.url, !allowLocalUrls)
const urlMeta = await getURLMeta(req.body.url, allowLocalUrls)
return res.json(urlMeta)
} catch (err) {
logger.error(err, 'controller.url.meta.error', req.id)
Expand All @@ -75,12 +73,12 @@ const get = async (req, res) => {
}

async function getSize () {
const { size } = await getURLMeta(req.body.url, !allowLocalUrls)
const { size } = await getURLMeta(req.body.url, allowLocalUrls)
return size
}

async function download () {
return downloadURL(req.body.url, !allowLocalUrls, req.id)
return downloadURL(req.body.url, allowLocalUrls, req.id)
}

try {
Expand Down
18 changes: 9 additions & 9 deletions packages/@uppy/companion/src/server/helpers/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ module.exports.validateURL = validateURL
/**
* Returns http Agent that will prevent requests to private IPs (to prevent SSRF)
*/
const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => {
const getProtectedHttpAgent = ({ protocol, allowLocalIPs }) => {
function dnsLookup (hostname, options, callback) {
dns.lookup(hostname, options, (err, addresses, maybeFamily) => {
if (err) {
Expand All @@ -58,7 +58,7 @@ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => {
// 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))
const validAddresses = allowLocalIPs ? toValidate : toValidate.filter(({ address }) => !isDisallowedIP(address))

// and check if there's anything left after we filtered:
if (validAddresses.length === 0) {
Expand All @@ -73,7 +73,7 @@ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => {

return class HttpAgent extends (protocol.startsWith('https') ? https : http).Agent {
createConnection (options, callback) {
if (ipaddr.isValid(options.host) && blockLocalIPs && isDisallowedIP(options.host)) {
if (ipaddr.isValid(options.host) && !allowLocalIPs && isDisallowedIP(options.host)) {
callback(new Error(FORBIDDEN_IP_ADDRESS))
return undefined
}
Expand All @@ -85,9 +85,9 @@ const getProtectedHttpAgent = ({ protocol, blockLocalIPs }) => {

module.exports.getProtectedHttpAgent = getProtectedHttpAgent

async function getProtectedGot ({ blockLocalIPs }) {
const HttpAgent = getProtectedHttpAgent({ protocol: 'http', blockLocalIPs })
const HttpsAgent = getProtectedHttpAgent({ protocol: 'https', blockLocalIPs })
async function getProtectedGot ({ allowLocalIPs }) {
const HttpAgent = getProtectedHttpAgent({ protocol: 'http', allowLocalIPs })
const HttpsAgent = getProtectedHttpAgent({ protocol: 'https', allowLocalIPs })
const httpAgent = new HttpAgent()
const httpsAgent = new HttpsAgent()

Expand All @@ -102,12 +102,12 @@ module.exports.getProtectedGot = getProtectedGot
* Gets the size and content type of a url's content
*
* @param {string} url
* @param {boolean} blockLocalIPs
* @param {boolean} allowLocalIPs
* @returns {Promise<{name: string, type: string, size: number}>}
*/
exports.getURLMeta = async (url, blockLocalIPs = false) => {
exports.getURLMeta = async (url, allowLocalIPs = false) => {
async function requestWithMethod (method) {
const protectedGot = await getProtectedGot({ blockLocalIPs })
const protectedGot = await getProtectedGot({ allowLocalIPs })
const stream = protectedGot.stream(url, { method, throwHttpErrors: false })

return new Promise((resolve, reject) => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class Facebook extends Provider {
async size ({ id, token }) {
return this.#withErrorHandling('provider.facebook.size.error', async () => {
const url = await getMediaUrl({ token, id })
const { size } = await getURLMeta(url, true)
const { size } = await getURLMeta(url)
return size
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class Instagram extends Provider {
async size ({ id, token }) {
return this.#withErrorHandling('provider.instagram.size.error', async () => {
const url = await getMediaUrl({ token, id })
const { size } = await getURLMeta(url, true)
const { size } = await getURLMeta(url)
return size
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class Unsplash extends Provider {
async size ({ id, token }) {
return this.#withErrorHandling('provider.unsplash.size.error', async () => {
const { links: { download: url } } = await getPhotoMeta(await getClient({ token }), id)
const { size } = await getURLMeta(url, true)
const { size } = await getURLMeta(url)
return size
})
}
Expand Down
12 changes: 6 additions & 6 deletions packages/@uppy/companion/test/__tests__/http-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,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'
return (await getProtectedGot({ blockLocalIPs: true })).get(url)
return (await getProtectedGot({ allowLocalIPs: false })).get(url)
})

test('blocks url that resolves to forbidden IP', async () => {
const url = 'https://localhost'
const promise = getProtectedGot({ blockLocalIPs: true }).then(got => got.get(url))
const promise = getProtectedGot({ allowLocalIPs: false }).then(got => got.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({ blockLocalIPs: true }).then(got => got.get(url))
const promise = getProtectedGot({ allowLocalIPs: false }).then(got => got.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({ blockLocalIPs: true }).then(got => got.get(url))
const promise = getProtectedGot({ allowLocalIPs: false }).then(got => got.get(url))
await expect(promise).rejects.toThrow(new Error(FORBIDDEN_IP_ADDRESS))
})

Expand Down Expand Up @@ -57,12 +57,12 @@ describe('test protected request Agent', () => {

for (const ip of ipv4s) {
const url = `http://${ip}:8090`
const promise = getProtectedGot({ blockLocalIPs: true }).then(got => got.get(url))
const promise = getProtectedGot({ allowLocalIPs: false }).then(got => got.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({ blockLocalIPs: true }).then(got => got.get(url))
const promise = getProtectedGot({ allowLocalIPs: false }).then(got => got.get(url))
await expect(promise).rejects.toThrow(new Error(FORBIDDEN_IP_ADDRESS))
}
})
Expand Down

0 comments on commit 1b1e5c7

Please sign in to comment.