From d3c641ea31a0767216970750115975b0c4416814 Mon Sep 17 00:00:00 2001 From: Sebastian Gierlinger Date: Fri, 14 Feb 2014 11:00:11 +0100 Subject: [PATCH] Make session expiry less arsey closes #2171 - added authentication middleware - removed authentication from routes - moved authentication before CSRF validation - moved caching rules before authentication - changed/added test --- core/server/middleware/index.js | 14 ++++++---- core/server/middleware/middleware.js | 22 +++++++++++++++ core/server/routes/admin.js | 19 +++++++------ core/server/routes/api.js | 34 +++++++++++------------ core/test/functional/admin/login_test.js | 2 +- core/test/functional/api/error_test.js | 20 +++++++++++++ core/test/functional/api/posts_test.js | 16 +++++++++++ core/test/functional/api/settings_test.js | 16 +++++++++++ core/test/functional/api/users_test.js | 16 +++++++++++ core/test/functional/routes/admin_test.js | 4 +-- 10 files changed, 128 insertions(+), 35 deletions(-) create mode 100644 core/test/functional/api/error_test.js diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 8bc11407c4..0ba10c3594 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -267,6 +267,14 @@ module.exports = function (server, dbHash) { cookie: cookie })); + // ### Caching + expressServer.use(middleware.cacheControl('public')); + expressServer.use('/api/', middleware.cacheControl('private')); + expressServer.use('/ghost/', middleware.cacheControl('private')); + + // enable authentication; has to be done before CSRF handling + expressServer.use(middleware.authenticate); + // enable express csrf protection expressServer.use(middleware.conditionalCSRF); @@ -277,12 +285,6 @@ module.exports = function (server, dbHash) { // Initialise the views expressServer.use(initViews); - - // ### Caching - expressServer.use(middleware.cacheControl('public')); - expressServer.use('/api/', middleware.cacheControl('private')); - expressServer.use('/ghost/', middleware.cacheControl('private')); - // ### Routing expressServer.use(subdir, expressServer.router); diff --git a/core/server/middleware/middleware.js b/core/server/middleware/middleware.js index 39a0bbab5a..e5c71b9a21 100644 --- a/core/server/middleware/middleware.js +++ b/core/server/middleware/middleware.js @@ -24,6 +24,28 @@ function cacheServer(server) { var middleware = { + // ### Authenticate Middleware + // authentication has to be done for /ghost/* routes with + // exceptions for signin, signout, signup, forgotten, reset only + // api and frontend use different authentication mechanisms atm + authenticate: function (req, res, next) { + if (res.isAdmin) { + if (req.path.indexOf("/ghost/api/") === 0) { + return middleware.authAPI(req, res, next); + } + + var noAuthNeeded = [ + '/ghost/signin/', '/ghost/signout/', '/ghost/signup/', + '/ghost/forgotten/', '/ghost/reset/' + ]; + + if (noAuthNeeded.indexOf(req.path) < 0) { + return middleware.auth(req, res, next); + } + } + next(); + }, + // ### Auth Middleware // Authenticate a request by redirecting to login if not logged in. // We strip /ghost/ out of the redirect parameter for neatness diff --git a/core/server/routes/admin.js b/core/server/routes/admin.js index a87f15525b..76d9228917 100644 --- a/core/server/routes/admin.js +++ b/core/server/routes/admin.js @@ -35,23 +35,24 @@ module.exports = function (server) { server.post('/ghost/reset/:token', admin.resetPassword); server.post('/ghost/signin/', admin.auth); server.post('/ghost/signup/', admin.doRegister); - server.post('/ghost/changepw/', middleware.auth, admin.changepw); - server.get('/ghost/editor(/:id)/', middleware.auth, admin.editor); - server.get('/ghost/editor/', middleware.auth, admin.editor); - server.get('/ghost/content/', middleware.auth, admin.content); - server.get('/ghost/settings*', middleware.auth, admin.settings); - server.get('/ghost/debug/', middleware.auth, admin.debug.index); - server.post('/ghost/upload/', middleware.auth, middleware.busboy, admin.uploader); + server.post('/ghost/changepw/', admin.changepw); + server.get('/ghost/editor(/:id)/', admin.editor); + server.get('/ghost/editor/', admin.editor); + server.get('/ghost/content/', admin.content); + server.get('/ghost/settings*', admin.settings); + server.get('/ghost/debug/', admin.debug.index); + + server.post('/ghost/upload/', middleware.busboy, admin.uploader); // redirect to /ghost and let that do the authentication to prevent redirects to /ghost//admin etc. server.get(/\/((ghost-admin|admin|wp-admin|dashboard|signin)\/?)$/, function (req, res) { /*jslint unparam:true*/ res.redirect(subdir + '/ghost/'); }); - server.get(/\/(ghost$\/?)/, middleware.auth, function (req, res) { + server.get(/\/(ghost$\/?)/, function (req, res) { /*jslint unparam:true*/ res.redirect(subdir + '/ghost/'); }); - server.get('/ghost/', middleware.redirectToSignup, middleware.auth, admin.index); + server.get('/ghost/', admin.index); }; \ No newline at end of file diff --git a/core/server/routes/api.js b/core/server/routes/api.js index 9d1bfb54b1..ed2742fa3e 100644 --- a/core/server/routes/api.js +++ b/core/server/routes/api.js @@ -4,27 +4,27 @@ var middleware = require('../middleware').middleware, module.exports = function (server) { // ### API routes // #### Posts - server.get('/ghost/api/v0.1/posts', middleware.authAPI, api.requestHandler(api.posts.browse)); - server.post('/ghost/api/v0.1/posts', middleware.authAPI, api.requestHandler(api.posts.add)); - server.get('/ghost/api/v0.1/posts/:id', middleware.authAPI, api.requestHandler(api.posts.read)); - server.put('/ghost/api/v0.1/posts/:id', middleware.authAPI, api.requestHandler(api.posts.edit)); - server.del('/ghost/api/v0.1/posts/:id', middleware.authAPI, api.requestHandler(api.posts.destroy)); + server.get('/ghost/api/v0.1/posts', api.requestHandler(api.posts.browse)); + server.post('/ghost/api/v0.1/posts', api.requestHandler(api.posts.add)); + server.get('/ghost/api/v0.1/posts/:id', api.requestHandler(api.posts.read)); + server.put('/ghost/api/v0.1/posts/:id', api.requestHandler(api.posts.edit)); + server.del('/ghost/api/v0.1/posts/:id', api.requestHandler(api.posts.destroy)); server.get('/ghost/api/v0.1/posts/getSlug/:title', middleware.authAPI, api.requestHandler(api.posts.getSlug)); // #### Settings - server.get('/ghost/api/v0.1/settings/', middleware.authAPI, api.requestHandler(api.settings.browse)); - server.get('/ghost/api/v0.1/settings/:key/', middleware.authAPI, api.requestHandler(api.settings.read)); - server.put('/ghost/api/v0.1/settings/', middleware.authAPI, api.requestHandler(api.settings.edit)); + server.get('/ghost/api/v0.1/settings/', api.requestHandler(api.settings.browse)); + server.get('/ghost/api/v0.1/settings/:key/', api.requestHandler(api.settings.read)); + server.put('/ghost/api/v0.1/settings/', api.requestHandler(api.settings.edit)); // #### Users - server.get('/ghost/api/v0.1/users/', middleware.authAPI, api.requestHandler(api.users.browse)); - server.get('/ghost/api/v0.1/users/:id/', middleware.authAPI, api.requestHandler(api.users.read)); - server.put('/ghost/api/v0.1/users/:id/', middleware.authAPI, api.requestHandler(api.users.edit)); + server.get('/ghost/api/v0.1/users/', api.requestHandler(api.users.browse)); + server.get('/ghost/api/v0.1/users/:id/', api.requestHandler(api.users.read)); + server.put('/ghost/api/v0.1/users/:id/', api.requestHandler(api.users.edit)); // #### Tags - server.get('/ghost/api/v0.1/tags/', middleware.authAPI, api.requestHandler(api.tags.all)); + server.get('/ghost/api/v0.1/tags/', api.requestHandler(api.tags.all)); // #### Notifications - server.del('/ghost/api/v0.1/notifications/:id', middleware.authAPI, api.requestHandler(api.notifications.destroy)); - server.post('/ghost/api/v0.1/notifications/', middleware.authAPI, api.requestHandler(api.notifications.add)); + server.del('/ghost/api/v0.1/notifications/:id', api.requestHandler(api.notifications.destroy)); + server.post('/ghost/api/v0.1/notifications/', api.requestHandler(api.notifications.add)); // #### Import/Export - server.get('/ghost/api/v0.1/db/', middleware.auth, api.db.exportContent); - server.post('/ghost/api/v0.1/db/', middleware.authAPI, middleware.busboy, api.requestHandler(api.db.importContent)); - server.del('/ghost/api/v0.1/db/', middleware.authAPI, api.requestHandler(api.db.deleteAllContent)); + server.get('/ghost/api/v0.1/db/', api.db.exportContent); + server.post('/ghost/api/v0.1/db/', middleware.busboy, api.requestHandler(api.db.importContent)); + server.del('/ghost/api/v0.1/db/', api.requestHandler(api.db.deleteAllContent)); }; \ No newline at end of file diff --git a/core/test/functional/admin/login_test.js b/core/test/functional/admin/login_test.js index 10c8a39ccc..be2dfb07e8 100644 --- a/core/test/functional/admin/login_test.js +++ b/core/test/functional/admin/login_test.js @@ -41,7 +41,7 @@ CasperTest.begin("Ghost admin will load login page", 2, function suite(test) { CasperTest.begin('Redirects login to signin', 2, function suite(test) { casper.start(url + 'ghost/login/', function testRedirect(response) { test.assertEqual(response.status, 200, 'Response status should be 200.'); - test.assertUrlMatch(/ghost\/signin\/$/, 'Should be redirected to /signin/.'); + test.assertUrlMatch(/ghost\/signin\//, 'Should be redirected to /signin/.'); }); }, true); diff --git a/core/test/functional/api/error_test.js b/core/test/functional/api/error_test.js new file mode 100644 index 0000000000..62fb33ba74 --- /dev/null +++ b/core/test/functional/api/error_test.js @@ -0,0 +1,20 @@ +/*globals describe, before, after, beforeEach, afterEach, it */ +var testUtils = require('../../utils'), + should = require('should'), + _ = require('lodash'), + request = require('request'); + +describe('Unauthorized', function () { + + it('can\'t retrieve posts', function (done) { + request.get(testUtils.API.getApiURL('posts/'), function (error, response, body) { + response.should.have.status(401); + should.not.exist(response.headers['x-cache-invalidate']); + response.should.be.json; + var jsonResponse = JSON.parse(body); + jsonResponse.should.exist; + testUtils.API.checkResponseValue(jsonResponse, ['error']); + done(); + }); + }); +}); \ No newline at end of file diff --git a/core/test/functional/api/posts_test.js b/core/test/functional/api/posts_test.js index e82b1e3fd9..37af6cc524 100644 --- a/core/test/functional/api/posts_test.js +++ b/core/test/functional/api/posts_test.js @@ -247,6 +247,22 @@ describe('Post API', function () { }); }); }); + + it('can\'t edit a post with invalid CSRF token', function (done) { + request.get(testUtils.API.getApiURL('posts/1/'), function (error, response, body) { + var jsonResponse = JSON.parse(body), + changedValue = 'My new Title'; + jsonResponse.should.exist; + jsonResponse.title = changedValue; + + request.put({uri: testUtils.API.getApiURL('posts/1/'), + headers: {'X-CSRF-Token': 'invalid-token'}, + json: jsonResponse}, function (error, response, putBody) { + response.should.have.status(403); + done(); + }); + }); + }); }); // ## delete diff --git a/core/test/functional/api/settings_test.js b/core/test/functional/api/settings_test.js index e221031743..c7b3f363ab 100644 --- a/core/test/functional/api/settings_test.js +++ b/core/test/functional/api/settings_test.js @@ -100,6 +100,22 @@ describe('Settings API', function () { }); }); + it('can\'t edit settings with invalid CSRF token', function (done) { + request.get(testUtils.API.getApiURL('settings'), function (error, response, body) { + var jsonResponse = JSON.parse(body), + changedValue = 'Ghost changed'; + jsonResponse.should.exist; + jsonResponse.title = changedValue; + + request.put({uri: testUtils.API.getApiURL('settings/'), + headers: {'X-CSRF-Token': 'invalid-token'}, + json: jsonResponse}, function (error, response, putBody) { + response.should.have.status(403); + done(); + }); + }); + }); + it('can\'t edit non existent setting', function (done) { request.get(testUtils.API.getApiURL('settings'), function (error, response, body) { var jsonResponse = JSON.parse(body), diff --git a/core/test/functional/api/users_test.js b/core/test/functional/api/users_test.js index 1988a7eebc..dd710da108 100644 --- a/core/test/functional/api/users_test.js +++ b/core/test/functional/api/users_test.js @@ -100,4 +100,20 @@ describe('User API', function () { }); }); }); + + it('can\'t edit a user with invalid CSRF token', function (done) { + request.get(testUtils.API.getApiURL('users/me/'), function (error, response, body) { + var jsonResponse = JSON.parse(body), + changedValue = 'joe-bloggs.ghost.org'; + jsonResponse.should.exist; + jsonResponse.website = changedValue; + + request.put({uri: testUtils.API.getApiURL('users/me/'), + headers: {'X-CSRF-Token': 'invalid-token'}, + json: jsonResponse}, function (error, response, putBody) { + response.should.have.status(403); + done(); + }); + }); + }); }); diff --git a/core/test/functional/routes/admin_test.js b/core/test/functional/routes/admin_test.js index 450e978a77..0e9f626bf2 100644 --- a/core/test/functional/routes/admin_test.js +++ b/core/test/functional/routes/admin_test.js @@ -73,9 +73,9 @@ describe('Admin Routing', function () { }); }); - it('should redirect from /ghost to /ghost/signup when no user', function (done) { + it('should redirect from /ghost to /ghost/signin when no user', function (done) { request.get('/ghost/') - .expect('Location', /ghost\/signup/) + .expect('Location', /ghost\/signin/) .expect('Cache-Control', cacheRules['private']) .expect(302) .end(doEnd(done));