Skip to content

Commit

Permalink
Fix allowed origins (#5536)
Browse files Browse the repository at this point in the history
* fix typo

* escape regex in default companionAllowedHosts

- also fail early if invalid regex supplied
- and add unit tests

* Remove todo comment

Co-authored-by: Mikael Finstad <[email protected]>

---------

Co-authored-by: Merlijn Vos <[email protected]>
  • Loading branch information
mifi and Murderlon authored Dec 4, 2024
1 parent 684f661 commit 8ebdf73
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 40 deletions.
2 changes: 1 addition & 1 deletion .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ Authenticates and Uploads from Dropbox through Companion:

### Unit tests

Unit tests are using Jest and can be run with:
Unit tests are using Vitest and can be run with:

```bash
yarn test:unit
Expand Down
24 changes: 1 addition & 23 deletions packages/@uppy/companion-client/src/Provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
import type { UnknownProviderPlugin } from '@uppy/core/lib/Uppy.js'
import RequestClient, { authErrorStatusCode } from './RequestClient.ts'
import type { CompanionPluginOptions } from './index.ts'
import { isOriginAllowed } from './getAllowedHosts.ts'

export interface Opts extends PluginOpts, CompanionPluginOptions {
pluginId: string
Expand All @@ -28,29 +29,6 @@ function getOrigin() {
return location.origin
}

function getRegex(value?: string | RegExp) {
if (typeof value === 'string') {
return new RegExp(`^${value}$`)
}
if (value instanceof RegExp) {
return value
}
return undefined
}

function isOriginAllowed(
origin: string,
allowedOrigin: string | RegExp | Array<string | RegExp> | undefined,
) {
const patterns =
Array.isArray(allowedOrigin) ?
allowedOrigin.map(getRegex)
: [getRegex(allowedOrigin)]
return patterns.some(
(pattern) => pattern?.test(origin) || pattern?.test(`${origin}/`),
) // allowing for trailing '/'
}

export default class Provider<M extends Meta, B extends Body>
extends RequestClient<M, B>
implements CompanionClientProvider
Expand Down
48 changes: 48 additions & 0 deletions packages/@uppy/companion-client/src/getAllowedHosts.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { describe, it, expect } from 'vitest'
import getAllowedHosts, { isOriginAllowed } from './getAllowedHosts.ts'

describe('getAllowedHosts', () => {
it('can convert companionAllowedHosts', () => {
expect(getAllowedHosts('www.example.com', '')).toBe('www.example.com')
expect(
getAllowedHosts([/transloadit\.com/, 'www\\.example\\.com'], ''),
).toEqual([/transloadit\.com/, 'www\\.example\\.com'])
expect(() => getAllowedHosts(['['], '')).toThrow(
/^Invalid regular expression/,
)
})

it('can convert when companionAllowedHosts unset', () => {
expect(getAllowedHosts(undefined, 'http://server.com')).toBe(
'http:\\/\\/server\\.com',
)
expect(getAllowedHosts(undefined, 'https://server.com/')).toBe(
'https:\\/\\/server\\.com',
)
expect(getAllowedHosts(undefined, 'server.com')).toBe(
'https:\\/\\/server\\.com',
)
expect(getAllowedHosts(undefined, 'server.com/test')).toBe(
'https:\\/\\/server\\.com',
)
expect(getAllowedHosts(undefined, '//server.com:80/test')).toBe(
'https:\\/\\/server\\.com:80',
)
})
})

describe('isOriginAllowed', () => {
it('should check origin', () => {
expect(isOriginAllowed('a', [/^.+$/])).toBeTruthy()
expect(isOriginAllowed('a', ['^.+$'])).toBeTruthy()
expect(
isOriginAllowed('www.transloadit.com', ['www\\.transloadit\\.com']),
).toBeTruthy()
expect(
isOriginAllowed('www.transloadit.com', ['transloadit\\.com']),
).toBeFalsy()
expect(isOriginAllowed('match', ['fail', 'match'])).toBeTruthy()
// todo maybe next major:
// expect(isOriginAllowed('www.transloadit.com', ['\\.transloadit\\.com$'])).toBeTruthy()
})
})
73 changes: 57 additions & 16 deletions packages/@uppy/companion-client/src/getAllowedHosts.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,63 @@
// https://stackoverflow.com/a/3561711/6519037
function escapeRegex(string: string) {
return string.replace(/[/\-\\^$*+?.()|[\]{}]/g, '\\$&')
}

function wrapInRegex(value?: string | RegExp): RegExp | undefined {
if (typeof value === 'string') {
// TODO in the next major we should change this to `new RegExp(value)` so that the user can control start/end characters
return new RegExp(`^${value}$`) // throws if invalid regex
}
if (value instanceof RegExp) {
return value
}
return undefined
}

export default function getAllowedHosts(
hosts: string | RegExp | Array<string | RegExp> | undefined,
url: string,
companionAllowedHosts: string | RegExp | Array<string | RegExp> | undefined,
companionUrl: string,
): string | RegExp | Array<string | RegExp> {
if (hosts) {
if (
typeof hosts !== 'string' &&
!Array.isArray(hosts) &&
!(hosts instanceof RegExp)
) {
throw new TypeError(
`The option "companionAllowedHosts" must be one of string, Array, RegExp`,
)
if (companionAllowedHosts) {
const validate = (value: string | RegExp) => {
if (
!(typeof value === 'string' && wrapInRegex(value)) && // wrapInRegex throws if invalid regex
!(value instanceof RegExp)
) {
throw new TypeError(
`The option "companionAllowedHosts" must be one of string, Array, RegExp`,
)
}
}

if (Array.isArray(companionAllowedHosts)) {
companionAllowedHosts.every(validate)
} else {
validate(companionAllowedHosts)
}
return hosts
return companionAllowedHosts
}
// does not start with https://
if (/^(?!https?:\/\/).*$/i.test(url)) {
return `https://${url.replace(/^\/\//, '')}`

// if it does not start with https://, prefix it (and remove any leading slashes)
let ret = companionUrl
if (/^(?!https?:\/\/).*$/i.test(ret)) {
ret = `https://${companionUrl.replace(/^\/\//, '')}`
}
return new URL(url).origin
ret = new URL(ret).origin

ret = escapeRegex(ret)
return ret
}

export function isOriginAllowed(
origin: string,
allowedOrigin: string | RegExp | Array<string | RegExp> | undefined,
): boolean {
const patterns =
Array.isArray(allowedOrigin) ?
allowedOrigin.map(wrapInRegex)
: [wrapInRegex(allowedOrigin)]
return patterns.some(
(pattern) => pattern?.test(origin) || pattern?.test(`${origin}/`),
) // allowing for trailing '/'
}

0 comments on commit 8ebdf73

Please sign in to comment.