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.

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

@uppy/companion: remove oauthOrigin #5311

Merged
merged 7 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ COMPANION_DATADIR=./output
COMPANION_DOMAIN=localhost:3020
COMPANION_PROTOCOL=http
COMPANION_PORT=3020
COMPANION_CLIENT_ORIGINS=
COMPANION_CLIENT_ORIGINS=true
COMPANION_SECRET=development
COMPANION_PREAUTH_SECRET=development2
COMPANION_OAUTH_ORIGIN=*

# NOTE: Only enable this in development. Enabling it in production is a security risk
COMPANION_ALLOW_LOCAL_URLS=true
Expand Down
31 changes: 19 additions & 12 deletions docs/companion.md
Original file line number Diff line number Diff line change
Expand Up @@ -343,14 +343,6 @@ which has only the secret, nothing else.

:::

### `oauthOrigin` `COMPANION_OAUTH_ORIGIN` (required)

An [origin](https://developer.mozilla.org/en-US/docs/Glossary/Origin) specifying
allowed origins, or an array of origins (comma-separated origins in
`COMPANION_OAUTH_ORIGIN`). Any browser request from an origin that is not listed
will not receive OAuth2 tokens, and the OAuth request won’t complete. Set it to
`'*'` to allow all origins (not recommended).

#### `uploadUrls` `COMPANION_UPLOAD_URLS`

An allowlist (array) of strings (exact URLs) or regular expressions. Companion
Expand Down Expand Up @@ -643,12 +635,27 @@ risk.**

:::

#### `corsOrigins` `COMPANION_CLIENT_ORIGINS`
#### `corsOrigins` (required)

Allowed CORS Origins. Passed as the `origin` option in
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
[cors](https://github.com/expressjs/cors#configuration-options).

Note this is used for both CORS’ `Access-Control-Allow-Origin` header, and for
the
[`targetOrigin`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#targetorigin)
for `postMessage` calls in the context of OAuth.
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

Setting it to `true` treats any origin as a trusted one, making it easier to
impersonate your brand. Setting it to `false` disables cross-origin supports,
use this if you’re serving Companion and Uppy from the same domain name.

##### `COMPANION_CLIENT_ORIGINS`

Allowed CORS Origins (default `true`). Passed as the `origin` option in
[cors](https://github.com/expressjs/cors#configuration-options))
A comma-separated string of origins, or `'true'` (which will be interpreted as
the boolean value `true`), or `'false'` (which will be interpreted as the
boolean value `false`).

#### `COMPANION_CLIENT_ORIGINS_REGEX`
##### `COMPANION_CLIENT_ORIGINS_REGEX`

Like COMPANION_CLIENT_ORIGINS, but allows a single regex instead.
`COMPANION_CLIENT_ORIGINS` will be ignored if this is used. This is a
Expand Down
9 changes: 6 additions & 3 deletions docs/guides/migration-guides.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ These cover all the major Uppy versions and how to migrate to them.

- End-of-Life versions of Node.js are no longer supported (use latest 18.x LTS,
20.x LTS, or 22.x current).
- Setting the `oauthOrigin` option is now required. To get back to the unsafe
behavior of the previous version, set it to `'*'`.
- Setting the `corsOrigin` option is now required. You should define the list of
Copy link
Member

Choose a reason for hiding this comment

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

Let's link to this option in the docs

origins you expect your app to be served from, otherwise it can be
impersonated from a different origin you don’t control. Set it to `true` if
you don’t care about impersonating.
- `COMPANION_REDIS_EXPRESS_SESSION_PREFIX` now defaults to `companion-session:`
(before `sess:`). To revert keep backwards compatibility, set the environment
variable `COMPANION_REDIS_EXPRESS_SESSION_PREFIX=sess:`.
Expand Down Expand Up @@ -36,7 +38,8 @@ These cover all the major Uppy versions and how to migrate to them.
(inverted boolean).
- `downloadURL` 2nd (boolean) argument inverted.
- `StreamHttpJsonError` renamed to `HttpError`.
- Removed (undocumented) option `clients`.
- Removed the `oauthOrigin` option, as well as the (undocumented) option
`clients`. Use `corsOrigin` instead.

### `@uppy/companion-client`

Expand Down
2 changes: 1 addition & 1 deletion e2e/start-companion-with-load-balancer.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ const startCompanion = ({ name, port }) => {
COMPANION_ALLOW_LOCAL_URLS: 'true',
COMPANION_ENABLE_URL_ENDPOINT: 'true',
COMPANION_LOGGER_PROCESS_NAME: name,
COMPANION_OAUTH_ORIGIN: '*',
COMPANION_CLIENT_ORIGINS: 'true',
},
})
// Adding a `then` property so the return value is awaitable:
Expand Down
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/config/companion.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ const validateConfig = (companionOptions) => {
logger.error('Running without uploadUrls is a security risk and Companion will refuse to start up when running in production (NODE_ENV=production)', 'startup.uploadUrls')
}

if (!companionOptions.oauthOrigin) {
throw new TypeError('Option oauthOrigin is required. To disable security, pass "*"')
if (companionOptions.corsOrigins == null) {
throw new TypeError('Option corsOrigins is required. To disable security, pass true')
}

if (periodicPingUrls != null && (
Expand Down
117 changes: 82 additions & 35 deletions packages/@uppy/companion/src/server/controllers/connect.js
Original file line number Diff line number Diff line change
@@ -1,46 +1,30 @@
const oAuthState = require('../helpers/oauth-state')

const queryString = (params, prefix = '?') => {
const str = new URLSearchParams(params).toString()
return str ? `${prefix}${str}` : ''
}

/**
* initializes the oAuth flow for a provider.
*
* @param {object} req
* @param {object} res
* Derived from `cors` npm package.
* @see https://github.com/expressjs/cors/blob/791983ebc0407115bc8ae8e64830d440da995938/lib/index.js#L19-L34
* @param {string} origin
* @param {*} allowedOrigins
* @returns {boolean}
*/
module.exports = function connect(req, res) {
const { secret, oauthOrigin } = req.companion.options
const stateObj = oAuthState.generateState()

// not sure if we need to store origin in the session state (e.g. we could've just gotten it directly inside send-token)
// but we're afraid to change the logic there
if (!Array.isArray(oauthOrigin)) {
// If the server only allows a single origin, we ignore the client-supplied
// origin from query because we don't need it.
stateObj.origin = oauthOrigin
} else if (oauthOrigin.length < 2) {
// eslint-disable-next-line prefer-destructuring
stateObj.origin = oauthOrigin[0]
} else {
// If we have multiple allowed origins, we need to check the client-supplied origin from query.
// If the client provides an untrusted origin,
// we want to send `undefined`. `undefined` means `/`, which is the same origin when passed to `postMessage`.
// https://html.spec.whatwg.org/multipage/web-messaging.html#dom-window-postmessage-options-dev
const { origin } = JSON.parse(atob(req.query.state))
stateObj.origin = oauthOrigin.find(o => o === origin)
function isOriginAllowed(origin, allowedOrigins) {
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
if (Array.isArray(allowedOrigins)) {
return allowedOrigins.some(allowedOrigin => isOriginAllowed(origin, allowedOrigin))
}

if (req.companion.options.server.oauthDomain) {
stateObj.companionInstance = req.companion.buildURL('', true)
if (typeof allowedOrigins === 'string'){
return origin === allowedOrigins;
}
return allowedOrigins.test?.(origin) ?? !!allowedOrigins;
}

if (req.query.uppyPreAuthToken) {
stateObj.preAuthToken = req.query.uppyPreAuthToken
}

const queryString = (params, prefix = '?') => {
const str = new URLSearchParams(params).toString()
return str ? `${prefix}${str}` : ''
}

function encodeStateAndRedirect(req, res, stateObj) {
const { secret } = req.companion.options
const state = oAuthState.encodeState(stateObj, secret)
const { providerClass, providerGrantConfig } = req.companion

Expand All @@ -66,3 +50,66 @@ module.exports = function connect(req, res) {
// Now we redirect to grant's /connect endpoint, see `app.use(Grant(grantConfig))`
res.redirect(req.companion.buildURL(`/connect/${oauthProvider}${qs}`, true))
}

function getClientOrigin(base64EncodedState) {
try {
const { origin } = JSON.parse(atob(base64EncodedState))
return origin
} catch {
return undefined
}
}


/**
* Initializes the oAuth flow for a provider.
*
* The client has open a new tab and is about to be redirected to the auth
* provider. When the user will return to companion, we'll have to send the auth
* token back to Uppy with `window.postMessage()`.
* To prevent other tabs and unauthorized origins from accessing that token, we
* reuse origin(s) from `corsOrigins` to limit the scope of `postMessage()`, which
* has `targetOrigin` parameter, required for cross-origin messages (i.e. if Uppy
* and Companion are served from different origins).
* We support multiple origins in `corsOrigins`, we have to figure out which
* origin the current connect request is coming from. Because the OAuth window
* was opened with `window.open()`, starting a new browsing context, the request
* is not cross origin and we don't have a `Origin` header to work with.
* That's why we use the client-provided base64-encoded parameter, check if it
* matches origin(s) allowed in `corsOrigins` Companion option, and use that as
* our `targetOrigin` for the `window.postMessage()` call (see `send-token.js`).
*
* @param {object} req
* @param {object} res
*/
module.exports = function connect(req, res, next) {
const stateObj = oAuthState.generateState()

if (req.companion.options.server.oauthDomain) {
stateObj.companionInstance = req.companion.buildURL('', true)
}

if (req.query.uppyPreAuthToken) {
stateObj.preAuthToken = req.query.uppyPreAuthToken
}

// Get the computed header generated by `cors` in a previous middleware.
stateObj.origin = res.getHeader('Access-Control-Allow-Origin')
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
let clientOrigin
if (!stateObj.origin && (clientOrigin = getClientOrigin(req.query.state))) {
const { corsOrigins } = req.companion.options

if (typeof corsOrigins === 'function') {
corsOrigins(clientOrigin, (err, finalOrigin) => {
if (err) next(err)
stateObj.origin = finalOrigin
encodeStateAndRedirect(req, res, stateObj)
})
return
}
if (isOriginAllowed(clientOrigin, req.companion.options.corsOrigins)) {
stateObj.origin = clientOrigin
}
}
encodeStateAndRedirect(req, res, stateObj)
}
17 changes: 11 additions & 6 deletions packages/@uppy/companion/src/standalone/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,17 @@ const companionProtocol = process.env.COMPANION_PROTOCOL || 'http'

function getCorsOrigins () {
if (process.env.COMPANION_CLIENT_ORIGINS) {
return process.env.COMPANION_CLIENT_ORIGINS
.split(',')
.map((url) => (hasProtocol(url) ? url : `${companionProtocol}://${url}`))
switch (process.env.COMPANION_CLIENT_ORIGINS) {

case 'true': return true
case 'false': return false
case '*': return '*'

default:
return process.env.COMPANION_CLIENT_ORIGINS
.split(',')
.map((url) => (hasProtocol(url) ? url : `${companionProtocol}://${url}`))
}
}
if (process.env.COMPANION_CLIENT_ORIGINS_REGEX) {
return new RegExp(process.env.COMPANION_CLIENT_ORIGINS_REGEX)
Expand Down Expand Up @@ -183,9 +191,6 @@ const getConfigFromEnv = () => {
corsOrigins: getCorsOrigins(),
testDynamicOauthCredentials: process.env.COMPANION_TEST_DYNAMIC_OAUTH_CREDENTIALS === 'true',
testDynamicOauthCredentialsSecret: process.env.COMPANION_TEST_DYNAMIC_OAUTH_CREDENTIALS_SECRET,
oauthOrigin: process.env.COMPANION_OAUTH_ORIGIN?.includes(',') ?
process.env.COMPANION_OAUTH_ORIGIN.split(',') :
process.env.COMPANION_OAUTH_ORIGIN,
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/companion/test/__tests__/uploader.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ afterAll(() => {

process.env.COMPANION_DATADIR = './test/output'
process.env.COMPANION_DOMAIN = 'localhost:3020'
process.env.COMPANION_OAUTH_ORIGIN = '*'
process.env.COMPANION_CLIENT_ORIGINS = 'true'
const { companionOptions } = standalone()

const mockReq = {}
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/companion/test/mockserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const defaultEnv = {

COMPANION_ENABLE_URL_ENDPOINT: 'true',

COMPANION_OAUTH_ORIGIN: '*',
COMPANION_CLIENT_ORIGINS: 'true',
}

function updateEnv (env) {
Expand Down