Skip to content

Commit

Permalink
Merge pull request #4821 from markstos/https-subdirectory
Browse files Browse the repository at this point in the history 
Fixes #4435, also refactors checkSSL to be unit-tested.
  • Loading branch information
@sebgie
sebgie committed Feb 27, 2015
2 parents 2f04e85 + 770317b commit ab2c57e
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 38 deletions.
39 changes: 1 addition & 38 deletions core/server/middleware/index.js
View file
Expand Up @@ -16,7 +16,6 @@ var api = require('../api'),
routes = require('../routes'),
slashes = require('connect-slashes'),
storage = require('../storage'),
url = require('url'),
_ = require('lodash'),
passport = require('passport'),
oauth = require('./oauth'),
Expand Down Expand Up @@ -163,42 +162,6 @@ function uncapitalise(req, res, next) {
}
}

function isSSLrequired(isAdmin) {
var forceSSL = url.parse(config.url).protocol === 'https:' ? true : false,
forceAdminSSL = (isAdmin && config.forceAdminSSL);
if (forceSSL || forceAdminSSL) {
return true;
}
return false;
}

// Check to see if we should use SSL
// and redirect if needed
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.sendStatus(403);
}

redirectUrl = url.parse(config.urlSSL || config.url);
return res.redirect(301, url.format({
protocol: 'https:',
hostname: redirectUrl.hostname,
port: redirectUrl.port,
pathname: req.path,
query: req.query
}));
}
}
next();
}

// ### ServeSharedFile Middleware
// Handles requests to robots.txt and favicon.ico (and caches them)
function serveSharedFile(file, type, maxAge) {
Expand Down Expand Up @@ -287,7 +250,7 @@ setupMiddleware = function (blogAppInstance, adminApp) {
// NOTE: Importantly this is _after_ the check above for admin-theme static resources,
// which do not need HTTPS. In fact, if HTTPS is forced on them, then 404 page might
// not display properly when HTTPS is not available!
blogApp.use(checkSSL);
blogApp.use(middleware.checkSSL);
adminApp.set('views', config.paths.adminViews);

// Theme only config
Expand Down
70 changes: 70 additions & 0 deletions core/server/middleware/middleware.js
View file
Expand Up @@ -10,6 +10,7 @@ var _ = require('lodash'),
api = require('../api'),
passport = require('passport'),
errors = require('../errors'),
url = require('url'),
utils = require('../utils'),

middleware,
Expand All @@ -32,6 +33,49 @@ function cacheOauthServer(server) {
oauthServer = server;
}

function isSSLrequired(isAdmin, configUrl, forceAdminSSL) {
var forceSSL = url.parse(configUrl).protocol === 'https:' ? true : false;
if (forceSSL || (isAdmin && forceAdminSSL)) {
return true;
}
return false;
}

// The guts of checkSSL. Indicate forbidden or redirect according to configuration.
// Required args: forceAdminSSL, url and urlSSL should be passed from config. reqURL from req.url
function sslForbiddenOrRedirect(opt) {
var forceAdminSSL = opt.forceAdminSSL,
reqUrl = opt.reqUrl, // expected to be relative-to-root
baseUrl = url.parse(opt.configUrlSSL || opt.configUrl),
response = {
// Check if forceAdminSSL: { redirect: false } is set, which means
// we should just deny non-SSL access rather than redirect
isForbidden: (forceAdminSSL && forceAdminSSL.redirect !== undefined && !forceAdminSSL.redirect),

// Append the request path to the base configuration path, trimming out a double "//"
redirectPathname: function () {
var pathname = baseUrl.path;
if (reqUrl[0] === '/' && pathname[pathname.length - 1] === '/') {
pathname += reqUrl.slice(1);
} else {
pathname += reqUrl;
}
return pathname;
},
redirectUrl: function (query) {
return url.format({
protocol: 'https:',
hostname: baseUrl.hostname,
port: baseUrl.port,
pathname: this.redirectPathname(),
query: query
});
}
};

return response;
}

middleware = {

// ### Authenticate Middleware
Expand Down Expand Up @@ -259,9 +303,35 @@ middleware = {
return oauthServer.token()(req, res, next);
},

// Check to see if we should use SSL
// and redirect if needed
checkSSL: function (req, res, next) {
if (isSSLrequired(res.isAdmin, config.url, config.forceAdminSSL)) {
if (!req.secure) {
var response = sslForbiddenOrRedirect({
forceAdminSSL: config.forceAdminSSL,
configUrlSSL: config.urlSSL,
configUrl: config.url,
reqUrl: req.url
});

if (response.isForbidden) {
return res.sendStatus(403);
} else {
return res.redirect(301, response.redirectUrl(req.query));
}
}
}
next();
},

busboy: busboy
};

module.exports = middleware;
module.exports.cacheBlogApp = cacheBlogApp;
module.exports.cacheOauthServer = cacheOauthServer;

// SSL helper functions are exported primarily for unity testing.
module.exports.isSSLrequired = isSSLrequired;
module.exports.sslForbiddenOrRedirect = sslForbiddenOrRedirect;
66 changes: 66 additions & 0 deletions core/test/unit/middleware_spec.js
View file
Expand Up @@ -170,4 +170,70 @@ describe('Middleware', function () {
});
});
});

describe('isSSLRequired', function () {
var isSSLrequired = middleware.isSSLrequired;

it('SSL is required if config.url starts with https', function () {
isSSLrequired(undefined, 'https://example.com', undefined).should.be.true;
});

it('SSL is required if isAdmin and config.forceAdminSSL is set', function () {
isSSLrequired(true, 'http://example.com', true).should.be.true;
});

it('SSL is not required if config.url starts with "http:/" and forceAdminSSL is not set', function () {
isSSLrequired(false, 'http://example.com', false).should.be.false;
});
});

describe('sslForbiddenOrRedirect', function () {
var sslForbiddenOrRedirect = middleware.sslForbiddenOrRedirect;
it('Return forbidden if config forces admin SSL for AdminSSL redirect is false.', function () {
var response = sslForbiddenOrRedirect({
forceAdminSSL: {redirect: false},
configUrl: 'http://example.com'
});
response.isForbidden.should.be.true;
});

it('If not forbidden, should produce SSL to redirect to when config.url ends with no slash', function () {
var response = sslForbiddenOrRedirect({
forceAdminSSL: {redirect: true},
configUrl: 'http://example.com/config/path',
reqUrl: '/req/path'
});
response.isForbidden.should.be.false;
response.redirectUrl({}).should.equal('https://example.com/config/path/req/path');
});

it('If config ends is slash, potential double-slash in resulting URL is removed', function () {
var response = sslForbiddenOrRedirect({
forceAdminSSL: {redirect: true},
configUrl: 'http://example.com/config/path/',
reqUrl: '/req/path'
});
response.redirectUrl({}).should.equal('https://example.com/config/path/req/path');
});

it('If config.urlSSL is provided it is preferred over config.url', function () {
var response = sslForbiddenOrRedirect({
forceAdminSSL: {redirect: true},
configUrl: 'http://example.com/config/path/',
configUrlSSL: 'https://example.com/ssl/config/path/',
reqUrl: '/req/path'
});
response.redirectUrl({}).should.equal('https://example.com/ssl/config/path/req/path');
});

it('query string in request is preserved in redirect URL', function () {
var response = sslForbiddenOrRedirect({
forceAdminSSL: {redirect: true},
configUrl: 'http://example.com/config/path/',
configUrlSSL: 'https://example.com/ssl/config/path/',
reqUrl: '/req/path'
});
response.redirectUrl({a: 'b'}).should.equal('https://example.com/ssl/config/path/req/path?a=b');
});
});
});

0 comments on commit ab2c57e

Please sign in to comment.