Fix allowed origins (#5536)

* 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 <finstaden@gmail.com>

---------

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

.github/CONTRIBUTING.md

@@ -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

packages/@uppy/companion-client/src/Provider.ts

@@ -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
@@ -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

packages/@uppy/companion-client/src/getAllowedHosts.test.ts

@@ -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()
+  })
+})

packages/@uppy/companion-client/src/getAllowedHosts.ts

@@ -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 '/'
 }