From a01384050392f5d39bee4e2809a469ededc24af5 Mon Sep 17 00:00:00 2001 From: Lev Gimelfarb Date: Fri, 21 Feb 2014 20:25:31 -0500 Subject: [PATCH] Support for urlSSL config option and forceAdminSSL 403 response closes #1838 - adding `forceAdminSSL: {redirect: true/false}` option to allow 403 over non-SSL rather than redirect - adding `urlSSL` option to specify SSL variant of `url` - using `urlSSL` when redirecting to SSL (forceAdminSSL), if specified - dynamically patching `.url` property for view engine templates to use SSL variant over HTTPS connections (pass `.secure` property as view engine data) - using `urlSSL` in a "reset password" email, if specified - adding unit tests to test `forceAdminSSL` and `urlSSL` options - created a unit test utility function to dynamically fork a new instance of Ghost during the test, with different configuration options --- core/server/config/url.js | 16 ++- core/server/controllers/admin.js | 5 +- core/server/controllers/frontend.js | 22 +++- core/server/middleware/index.js | 30 ++++- core/test/functional/routes/admin_test.js | 74 +++++++++++ core/test/functional/routes/frontend_test.js | 42 ++++++ core/test/utils/fork.js | 128 +++++++++++++++++++ core/test/utils/index.js | 7 +- 8 files changed, 312 insertions(+), 12 deletions(-) create mode 100644 core/test/utils/fork.js diff --git a/core/server/config/url.js b/core/server/config/url.js index 1cf316c1e2..58b92b85db 100644 --- a/core/server/config/url.js +++ b/core/server/config/url.js @@ -26,17 +26,19 @@ function setConfig(config) { // Parameters: // - urlPath - string which must start and end with a slash // - absolute (optional, default:false) - boolean whether or not the url should be absolute +// - secure (optional, default:false) - boolean whether or not to use urlSSL or url config // Returns: // - a URL which always ends with a slash -function createUrl(urlPath, absolute) { +function createUrl(urlPath, absolute, secure) { urlPath = urlPath || '/'; absolute = absolute || false; - var output = ''; + var output = '', baseUrl; // create base of url, always ends without a slash if (absolute) { - output += ghostConfig.url.replace(/\/$/, ''); + baseUrl = (secure && ghostConfig.urlSSL) ? ghostConfig.urlSSL : ghostConfig.url; + output += baseUrl.replace(/\/$/, ''); } else { output += ghostConfig.paths.subdir; } @@ -99,6 +101,7 @@ function urlPathForPost(post, permalinks) { // This is probably not the right place for this, but it's the best place for now function urlFor(context, data, absolute) { var urlPath = '/', + secure, knownObjects = ['post', 'tag', 'user'], knownPaths = {'home': '/', 'rss': '/rss/'}; // this will become really big @@ -108,14 +111,19 @@ function urlFor(context, data, absolute) { data = null; } + // Can pass 'secure' flag in either context or data arg + secure = (context && context.secure) || (data && data.secure); + if (_.isObject(context) && context.relativeUrl) { urlPath = context.relativeUrl; } else if (_.isString(context) && _.indexOf(knownObjects, context) !== -1) { // trying to create a url for an object if (context === 'post' && data.post && data.permalinks) { urlPath = urlPathForPost(data.post, data.permalinks); + secure = data.post.secure; } else if (context === 'tag' && data.tag) { urlPath = '/tag/' + data.tag.slug + '/'; + secure = data.tag.secure; } // other objects are recognised but not yet supported } else if (_.isString(context) && _.indexOf(_.keys(knownPaths), context) !== -1) { @@ -123,7 +131,7 @@ function urlFor(context, data, absolute) { urlPath = knownPaths[context] || '/'; } - return createUrl(urlPath, absolute); + return createUrl(urlPath, absolute, secure); } // ## urlForPost diff --git a/core/server/controllers/admin.js b/core/server/controllers/admin.js index 6a31f08d52..dfe2a9ed10 100644 --- a/core/server/controllers/admin.js +++ b/core/server/controllers/admin.js @@ -301,8 +301,9 @@ adminControllers = { var email = req.body.email; api.users.generateResetToken(email).then(function (token) { - var siteLink = '' + config().url + '', - resetUrl = config().url.replace(/\/$/, '') + '/ghost/reset/' + token + '/', + var baseUrl = config().forceAdminSSL ? (config().urlSSL || config().url) : config().url, + siteLink = '' + baseUrl + '', + resetUrl = baseUrl.replace(/\/$/, '') + '/ghost/reset/' + token + '/', resetLink = '' + resetUrl + '', message = { to: email, diff --git a/core/server/controllers/frontend.js b/core/server/controllers/frontend.js index 1cb13ef92c..9af435500e 100644 --- a/core/server/controllers/frontend.js +++ b/core/server/controllers/frontend.js @@ -56,6 +56,14 @@ function handleError(next) { }; } +// Add Request context parameter to the data object +// to be passed down to the templates +function setReqCtx(req, data) { + (Array.isArray(data) ? data : [data]).forEach(function (d) { + d.secure = req.secure; + }); +} + frontendControllers = { 'homepage': function (req, res, next) { // Parse the page number @@ -76,6 +84,8 @@ frontendControllers = { return res.redirect(page.meta.pagination.pages === 1 ? config().paths.subdir + '/' : (config().paths.subdir + '/page/' + page.meta.pagination.pages + '/')); } + setReqCtx(req, page.posts); + // Render the page of posts filters.doFilter('prePostsRender', page.posts).then(function (posts) { res.render('index', formatPageResponse(posts, page)); @@ -113,6 +123,9 @@ frontendControllers = { return res.redirect(tagUrl(options.tag, page.meta.pagination.pages)); } + setReqCtx(req, page.posts); + setReqCtx(req, page.aspect.tag); + // Render the page of posts filters.doFilter('prePostsRender', page.posts).then(function (posts) { api.settings.read('activeTheme').then(function (activeTheme) { @@ -184,6 +197,9 @@ frontendControllers = { // Use throw 'no match' to show 404. throw new Error('no match'); } + + setReqCtx(req, post); + filters.doFilter('prePostsRender', post).then(function (post) { api.settings.read('activeTheme').then(function (activeTheme) { var paths = config().paths.availableThemes[activeTheme.value], @@ -279,8 +295,8 @@ frontendControllers = { var title = result[0].value.value, description = result[1].value.value, permalinks = result[2].value, - siteUrl = config.urlFor('home', null, true), - feedUrl = config.urlFor('rss', null, true), + siteUrl = config.urlFor('home', {secure: req.secure}, true), + feedUrl = config.urlFor('rss', {secure: req.secure}, true), maxPage = page.meta.pagination.pages, feedItems = [], feed; @@ -315,6 +331,8 @@ frontendControllers = { } } + setReqCtx(req, page.posts); + filters.doFilter('prePostsRender', page.posts).then(function (posts) { posts.forEach(function (post) { var deferred = when.defer(), diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 45ecfff399..77e6404fb0 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -70,13 +70,24 @@ function ghostLocals(req, res, next) { } } +function initThemeData(secure) { + var themeConfig = config.theme(); + if (secure && config().urlSSL) { + // For secure requests override .url property with the SSL version + themeConfig = _.clone(themeConfig); + themeConfig.url = config().urlSSL.replace(/\/$/, ''); + } + return themeConfig; +} + // ### InitViews Middleware // Initialise Theme or Admin Views function initViews(req, res, next) { /*jslint unparam:true*/ if (!res.isAdmin) { - hbs.updateTemplateOptions({ data: {blog: config.theme()} }); + var themeData = initThemeData(req.secure); + hbs.updateTemplateOptions({ data: {blog: themeData} }); expressServer.engine('hbs', expressServer.get('theme view engine')); expressServer.set('views', path.join(config().paths.themePath, expressServer.get('activeTheme'))); } else { @@ -84,6 +95,10 @@ function initViews(req, res, next) { expressServer.set('views', config().paths.adminViews); } + // Pass 'secure' flag to the view engine + // so that templates can choose 'url' vs 'urlSSL' + res.locals.secure = req.secure; + next(); } @@ -184,9 +199,20 @@ function isSSLrequired(isAdmin) { function checkSSL(req, res, next) { if (isSSLrequired(res.isAdmin)) { if (!req.secure) { + var forceAdminSSL = config().forceAdminSSL, + redirectUrl; + + // Check if forceAdminSSL: { redirect: false } is set, which means + // we should just deny non-SSL access rather than redirect + if (forceAdminSSL && forceAdminSSL.redirect !== undefined && !forceAdminSSL.redirect) { + return res.send(403); + } + + redirectUrl = url.parse(config().urlSSL || config().url); return res.redirect(301, url.format({ protocol: 'https:', - hostname: url.parse(config().url).hostname, + hostname: redirectUrl.hostname, + port: redirectUrl.port, pathname: req.path, query: req.query })); diff --git a/core/test/functional/routes/admin_test.js b/core/test/functional/routes/admin_test.js index a3725acf84..5c437ba7e6 100644 --- a/core/test/functional/routes/admin_test.js +++ b/core/test/functional/routes/admin_test.js @@ -111,6 +111,80 @@ describe('Admin Routing', function () { .end(doEndNoAuth(done)); }); }); + + // we'll use X-Forwarded-Proto: https to simulate an 'https://' request behind a proxy + describe('Require HTTPS - no redirect', function() { + var forkedGhost, request; + before(function (done) { + var configTestHttps = testUtils.fork.config(); + configTestHttps.forceAdminSSL = {redirect: false}; + configTestHttps.urlSSL = 'https://localhost/'; + + testUtils.fork.ghost(configTestHttps, 'testhttps') + .then(function(child) { + forkedGhost = child; + request = require('supertest'); + request = request(configTestHttps.url.replace(/\/$/, '')); + }, done) + .then(done); + }); + + after(function (done) { + if (forkedGhost) { + forkedGhost.kill(done); + } + }); + + it('should block admin access over non-HTTPS', function(done) { + request.get('/ghost/') + .expect(403) + .end(done); + }); + + it('should allow admin access over HTTPS', function(done) { + request.get('/ghost/signup/') + .set('X-Forwarded-Proto', 'https') + .expect(200) + .end(doEnd(done)); + }); + }); + + describe('Require HTTPS - redirect', function() { + var forkedGhost, request; + before(function (done) { + var configTestHttps = testUtils.fork.config(); + configTestHttps.forceAdminSSL = {redirect: true}; + configTestHttps.urlSSL = 'https://localhost/'; + + testUtils.fork.ghost(configTestHttps, 'testhttps') + .then(function(child) { + forkedGhost = child; + request = require('supertest'); + request = request(configTestHttps.url.replace(/\/$/, '')); + }, done) + .then(done); + }); + + after(function (done) { + if (forkedGhost) { + forkedGhost.kill(done); + } + }); + + it('should redirect admin access over non-HTTPS', function(done) { + request.get('/ghost/') + .expect('Location', /^https:\/\/localhost\/ghost\//) + .expect(301) + .end(done); + }); + + it('should allow admin access over HTTPS', function(done) { + request.get('/ghost/signup/') + .set('X-Forwarded-Proto', 'https') + .expect(200) + .end(done); + }); + }); describe('Ghost Admin Signup', function () { it('should have a session cookie which expires in 12 hours', function (done) { diff --git a/core/test/functional/routes/frontend_test.js b/core/test/functional/routes/frontend_test.js index f14934eff5..efd70d60fc 100644 --- a/core/test/functional/routes/frontend_test.js +++ b/core/test/functional/routes/frontend_test.js @@ -9,6 +9,7 @@ var request = require('supertest'), express = require('express'), should = require('should'), moment = require('moment'), + path = require('path'), testUtils = require('../../utils'), ghost = require('../../../../core'), @@ -142,6 +143,47 @@ describe('Frontend Routing', function () { .end(doEnd(done)); }); }); + + // we'll use X-Forwarded-Proto: https to simulate an 'https://' request behind a proxy + describe('HTTPS', function() { + var forkedGhost, request; + before(function (done) { + var configTestHttps = testUtils.fork.config(); + configTestHttps.forceAdminSSL = {redirect: false}; + configTestHttps.urlSSL = 'https://localhost/'; + + testUtils.fork.ghost(configTestHttps, 'testhttps') + .then(function(child) { + forkedGhost = child; + request = require('supertest'); + request = request(configTestHttps.url.replace(/\/$/, '')); + }, done) + .then(done); + }); + + after(function (done) { + if (forkedGhost) { + forkedGhost.kill(done); + } + }); + + it('should set links to url over non-HTTPS', function(done) { + request.get('/') + .expect(200) + .expect(/\/) + .expect(/copyright \Ghost\<\/a\>/) + .end(doEnd(done)); + }); + + it('should set links to urlSSL over HTTPS', function(done) { + request.get('/') + .set('X-Forwarded-Proto', 'https') + .expect(200) + .expect(/\/) + .expect(/copyright \Ghost\<\/a\>/) + .end(doEnd(done)); + }); + }); describe('RSS', function () { it('should redirect without slash', function (done) { diff --git a/core/test/utils/fork.js b/core/test/utils/fork.js new file mode 100644 index 0000000000..2d0825ae15 --- /dev/null +++ b/core/test/utils/fork.js @@ -0,0 +1,128 @@ +var cp = require('child_process'), + _ = require('lodash'), + fs = require('fs'), + url = require('url'), + net = require('net'), + when = require('when'), + path = require('path'), + config = require('../../server/config'); + +function findFreePort(port) { + var deferred = when.defer(); + + if (typeof port === 'string') port = parseInt(port); + if (typeof port !== 'number') port = 2368; + port = port + 1; + + var server = net.createServer(); + server.on('error', function(e) { + if (e.code === 'EADDRINUSE') { + when.chain(findFreePort(port), deferred); + } else { + deferred.reject(e); + } + }); + server.listen(port, function() { + var listenPort = server.address().port; + server.close(function() { + deferred.resolve(listenPort); + }); + }); + + return deferred.promise; +} + +// Get a copy of current config object from file, to be modified before +// passing to forkGhost() method +function forkConfig() { + // require caches values, and we want to read it fresh from the file + delete require.cache[config().paths.config]; + return _.cloneDeep(require(config().paths.config)[process.env.NODE_ENV]); +} + +// Creates a new fork of Ghost process with a given config +// Useful for tests that want to verify certain config options +function forkGhost(newConfig, envName) { + var deferred = when.defer(); + envName = envName || 'forked'; + findFreePort(newConfig.server ? newConfig.server.port : undefined) + .then(function(port) { + newConfig.server = newConfig.server || {}; + newConfig.server.port = port; + newConfig.url = url.format(_.extend(url.parse(newConfig.url), {port: port, host: null})); + + var newConfigFile = path.join(config().paths.appRoot, 'config.test' + port + '.js'); + fs.writeFile(newConfigFile, 'module.exports = {' + envName + ': ' + JSON.stringify(newConfig) + '}', function(err) { + if (err) throw err; + + // setup process environment for the forked Ghost to use the new config file + var env = _.clone(process.env); + env['GHOST_CONFIG'] = newConfigFile; + env['NODE_ENV'] = envName; + var child = cp.fork(path.join(config().paths.appRoot, 'index.js'), {env: env}); + + var pingTries = 0; + var pingCheck; + var pingStop = function() { + if (pingCheck) { + clearInterval(pingCheck); + pingCheck = undefined; + return true; + } + return false; + }; + // periodic check until forked Ghost is running and is listening on the port + pingCheck = setInterval(function() { + var socket = net.connect(port); + socket.on('connect', function() { + socket.end(); + if (pingStop()) { + deferred.resolve(child); + } + }); + socket.on('error', function(err) { + // continue checking + if (++pingTries >= 20 && pingStop()) { + deferred.reject(new Error("Timed out waiting for child process")); + } + }); + }, 200); + + child.on('exit', function(code, signal) { + child.exited = true; + if (pingStop()) { + deferred.reject(new Error("Child process exit code: " + code)); + } + // cleanup the temporary config file + fs.unlink(newConfigFile); + }); + + // override kill() to have an async callback + var baseKill = child.kill; + child.kill = function(signal, cb) { + if (typeof signal === 'function') { + cb = signal; + signal = undefined; + } + + if (cb) { + child.on('exit', function() { + cb(); + }); + } + + if (child.exited) { + process.nextTick(cb); + } else { + baseKill.apply(child, [signal]); + } + }; + }); + }) + .otherwise(deferred.reject); + + return deferred.promise; +} + +module.exports.ghost = forkGhost; +module.exports.config = forkConfig; diff --git a/core/test/utils/index.js b/core/test/utils/index.js index aae3913809..08d0d73fc1 100644 --- a/core/test/utils/index.js +++ b/core/test/utils/index.js @@ -8,7 +8,8 @@ var knex = require('../../server/models/base').knex, migration = require("../../server/data/migration/"), Settings = require('../../server/models/settings').Settings, DataGenerator = require('./fixtures/data-generator'), - API = require('./api'); + API = require('./api'), + fork = require('./fork'); function initData() { return migration.init(); @@ -228,5 +229,7 @@ module.exports = { loadExportFixture: loadExportFixture, DataGenerator: DataGenerator, - API: API + API: API, + + fork: fork };