From 2cd8f8993308a897a1314b137bbe4a8492ae602f Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 4 Dec 2019 13:06:30 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20500=20errors=20for=20inc?= =?UTF-8?q?orrect=20Origin=20headers=20(#11433)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no-issue Our function for determining cors options created a new instance of URL without wrapping it in a try/catch which meant any failures to parse the URL bubbled down as a 500 error. 500 errors are commonly used for alerting at the infrastructure level, and this error is definitely one caused by a badly configured client, so we wrap the construction and crap out with a Bad Request Error (HTTP 400) if it fails. --- core/server/web/site/app.js | 48 +++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/core/server/web/site/app.js b/core/server/web/site/app.js index 21b166e783..e8bad9709c 100644 --- a/core/server/web/site/app.js +++ b/core/server/web/site/app.js @@ -3,6 +3,7 @@ const path = require('path'); const express = require('express'); const cors = require('cors'); const {URL} = require('url'); +const common = require('../../lib/common'); // App requires const config = require('../../config'); @@ -30,27 +31,38 @@ const corsOptionsDelegate = function corsOptionsDelegate(req, callback) { credentials: true // required to allow admin-client to login to private sites }; - if (origin) { - const originUrl = new URL(origin); + if (!origin) { + return callback(null, corsOptions); + } - // allow all localhost and 127.0.0.1 requests no matter the port - if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1') { + let originUrl; + try { + originUrl = new URL(origin); + } catch (err) { + return callback(new common.errors.BadRequestError({err})); + } + + // originUrl will definitely exist here because according to WHATWG URL spec + // The class constructor will either throw a TypeError or return a URL object + // https://url.spec.whatwg.org/#url-class + + // allow all localhost and 127.0.0.1 requests no matter the port + if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1') { + corsOptions.origin = true; + } + + // allow the configured host through on any protocol + const siteUrl = new URL(config.get('url')); + if (originUrl.host === siteUrl.host) { + corsOptions.origin = true; + } + + // allow the configured admin:url host through on any protocol + if (config.get('admin:url')) { + const adminUrl = new URL(config.get('admin:url')); + if (originUrl.host === adminUrl.host) { corsOptions.origin = true; } - - // allow the configured host through on any protocol - const siteUrl = new URL(config.get('url')); - if (originUrl.host === siteUrl.host) { - corsOptions.origin = true; - } - - // allow the configured admin:url host through on any protocol - if (config.get('admin:url')) { - const adminUrl = new URL(config.get('admin:url')); - if (originUrl.host === adminUrl.host) { - corsOptions.origin = true; - } - } } callback(null, corsOptions);