From a701ee70239fb77943ab878ec87615d776218daa Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Mon, 6 Apr 2020 11:49:14 +0200 Subject: [PATCH] Added support for token session to /ghost (#11709) no-issue * Added default for getting origin of request This function is used to attach the origin of the request to the session, and later check that requests using the session are coming from the same origin. This protects us against CSRF attacks as requests in the browser MUST originate from the same origin on which the user logged in. Previously, when we could not determine the origin we would return null, as a "safety" net. This updates the function to use a secure and sensible default - which is the origin of the Ghost-Admin application, and if that's not set - the origin of the Ghost application. This will make dealing with magic links simpler as you can not always guaruntee the existence of these headers when visiting via a hyperlink * Removed init fns and getters from session service This simplifies the code here, making it easier to read and maintain * Moved express-session initialisation to own file This is complex enough that it deserves its own module * Added createSessionFromToken to session service * Wired up the createSessionFromToken middleware --- .../services/auth/session/express-session.js | 37 +++++ core/server/services/auth/session/index.js | 128 ++++-------------- core/server/web/parent-app.js | 2 +- package.json | 1 + test/unit/api/canary/session_spec.js | 2 +- test/unit/api/v2/session_spec.js | 2 +- test/unit/api/v3/session_spec.js | 2 +- .../services/auth/session/middleware_spec.js | 24 +--- yarn.lock | 5 + 9 files changed, 78 insertions(+), 125 deletions(-) create mode 100644 core/server/services/auth/session/express-session.js diff --git a/core/server/services/auth/session/express-session.js b/core/server/services/auth/session/express-session.js new file mode 100644 index 000000000000..52de15d5d728 --- /dev/null +++ b/core/server/services/auth/session/express-session.js @@ -0,0 +1,37 @@ +const session = require('express-session'); +const constants = require('../../../lib/constants'); +const config = require('../../../config'); +const settingsCache = require('../../settings/cache'); +const models = require('../../../models'); +const urlUtils = require('../../../lib/url-utils'); + +const SessionStore = require('./store'); + +const expressSessionMiddleware = session({ + store: new SessionStore(models.Session), + secret: settingsCache.get('session_secret'), + resave: false, + saveUninitialized: false, + name: 'ghost-admin-api-session', + cookie: { + maxAge: constants.SIX_MONTH_MS, + httpOnly: true, + path: urlUtils.getSubdir() + '/ghost', + sameSite: 'lax', + secure: urlUtils.isSSL(config.get('url')) + } +}); + +exports.getSession = async function getSession(req, res) { + if (req.session) { + return req.session; + } + return new Promise((resolve, reject) => { + expressSessionMiddleware(req, res, function (err) { + if (err) { + return reject(err); + } + resolve(req.session); + }); + }); +}; diff --git a/core/server/services/auth/session/index.js b/core/server/services/auth/session/index.js index 31e466cc8c8f..2d727f0b48e1 100644 --- a/core/server/services/auth/session/index.js +++ b/core/server/services/auth/session/index.js @@ -1,18 +1,17 @@ -const session = require('express-session'); -const constants = require('../../../lib/constants'); -const config = require('../../../config'); -const settingsCache = require('../../settings/cache'); +const adapterManager = require('../../adapter-manager'); +const createSessionService = require('@tryghost/session-service'); +const sessionFromToken = require('@tryghost/mw-session-from-token'); +const createSessionMiddleware = require('./middleware'); + +const expressSession = require('./express-session'); + const models = require('../../../models'); const urlUtils = require('../../../lib/url-utils'); const url = require('url'); -const SessionService = require('@tryghost/session-service'); -const SessionMiddleware = require('./middleware'); -const SessionStore = require('./store'); - function getOriginOfRequest(req) { const origin = req.get('origin'); - const referrer = req.get('referrer'); + const referrer = req.get('referrer') || urlUtils.getAdminUrl() || urlUtils.getSiteUrl(); if (!origin && !referrer) { return null; @@ -29,95 +28,22 @@ function getOriginOfRequest(req) { return null; } -async function getSession(req, res) { - if (req.session) { - return req.session; - } - return new Promise((resolve, reject) => { - expressSessionMiddleware(req, res, function (err) { - if (err) { - return reject(err); - } - resolve(req.session); - }); - }); -} - -function findUserById({id}) { - return models.User.findOne({id}); -} - -let expressSessionMiddleware; -function initExpressSessionMiddleware() { - if (!expressSessionMiddleware) { - expressSessionMiddleware = session({ - store: new SessionStore(models.Session), - secret: settingsCache.get('session_secret'), - resave: false, - saveUninitialized: false, - name: 'ghost-admin-api-session', - cookie: { - maxAge: constants.SIX_MONTH_MS, - httpOnly: true, - path: urlUtils.getSubdir() + '/ghost', - sameSite: 'lax', - secure: urlUtils.isSSL(config.get('url')) - } - }); - } -} - -let sessionService; -function initSessionService() { - if (!sessionService) { - if (!expressSessionMiddleware) { - initExpressSessionMiddleware(); - } - - sessionService = SessionService({ - getOriginOfRequest, - getSession, - findUserById - }); - } -} - -let sessionMiddleware; -function initSessionMiddleware() { - if (!sessionMiddleware) { - if (!sessionService) { - initSessionService(); - } - sessionMiddleware = SessionMiddleware({ - sessionService - }); - } -} - -module.exports = { - get createSession() { - return this.middleware.createSession; - }, - - get destroySession() { - return this.middleware.destroySession; - }, - - get authenticate() { - return this.middleware.authenticate; - }, - - get service() { - if (!sessionService) { - initSessionService(); - } - return sessionService; - }, - - get middleware() { - if (!sessionMiddleware) { - initSessionMiddleware(); - } - return sessionMiddleware; - } -}; +const sessionService = createSessionService({ + getOriginOfRequest, + getSession: expressSession.getSession, + findUserById({id}) { + return models.User.findOne({id}); + } +}); + +module.exports = createSessionMiddleware({sessionService}); + +const ssoAdapter = adapterManager.getAdapter('sso'); +// Looks funky but this is a "custom" piece of middleware +module.exports.createSessionFromToken = sessionFromToken({ + callNextWithError: false, + createSession: sessionService.createSessionForUser, + findUserByLookup: ssoAdapter.getUserForIdentity, + getLookupFromToken: ssoAdapter.getIdentityFromCredentials, + getTokenFromRequest: ssoAdapter.getRequestCredentials +}); diff --git a/core/server/web/parent-app.js b/core/server/web/parent-app.js index cb8264170cec..e565adfc985e 100644 --- a/core/server/web/parent-app.js +++ b/core/server/web/parent-app.js @@ -59,7 +59,7 @@ module.exports = function setupParentApp(options = {}) { adminApp.enable('trust proxy'); // required to respect x-forwarded-proto in admin requests adminApp.use('/ghost/api', require('./api')()); adminApp.use('/ghost/.well-known', require('./well-known')()); - adminApp.use('/ghost', require('./admin')()); + adminApp.use('/ghost', require('../services/auth/session').createSessionFromToken, require('./admin')()); // TODO: remove {admin url}/content/* once we're sure the API is not returning relative asset URLs anywhere // only register this route if the admin is separate so we're not overriding the {site}/content/* route diff --git a/package.json b/package.json index 464d4201fadd..038fd9f0fee8 100644 --- a/package.json +++ b/package.json @@ -50,6 +50,7 @@ "@tryghost/kg-markdown-html-renderer": "1.0.2", "@tryghost/members-api": "0.18.0", "@tryghost/members-ssr": "0.7.4", + "@tryghost/mw-session-from-token": "^0.1.0", "@tryghost/session-service": "^0.1.0", "@tryghost/social-urls": "0.1.8", "@tryghost/string": "0.1.8", diff --git a/test/unit/api/canary/session_spec.js b/test/unit/api/canary/session_spec.js index a48b4b6ad6d0..ae37dd67d0cd 100644 --- a/test/unit/api/canary/session_spec.js +++ b/test/unit/api/canary/session_spec.js @@ -5,7 +5,7 @@ const models = require('../../../../core/server/models'); const {UnauthorizedError} = require('../../../../core/server/lib/common/errors'); const sessionController = require('../../../../core/server/api/canary/session'); -const sessionServiceMiddleware = require('../../../../core/server/services/auth/session').middleware; +const sessionServiceMiddleware = require('../../../../core/server/services/auth/session'); describe('Session controller', function () { before(function () { diff --git a/test/unit/api/v2/session_spec.js b/test/unit/api/v2/session_spec.js index 8ff08d2cef46..0dff56ede18d 100644 --- a/test/unit/api/v2/session_spec.js +++ b/test/unit/api/v2/session_spec.js @@ -5,7 +5,7 @@ const models = require('../../../../core/server/models'); const {UnauthorizedError} = require('../../../../core/server/lib/common/errors'); const sessionController = require('../../../../core/server/api/v2/session'); -const sessionServiceMiddleware = require('../../../../core/server/services/auth/session').middleware; +const sessionServiceMiddleware = require('../../../../core/server/services/auth/session'); describe('v2 Session controller', function () { before(function () { diff --git a/test/unit/api/v3/session_spec.js b/test/unit/api/v3/session_spec.js index 6b4906d718fc..99781593783c 100644 --- a/test/unit/api/v3/session_spec.js +++ b/test/unit/api/v3/session_spec.js @@ -5,7 +5,7 @@ const models = require('../../../../core/server/models'); const {UnauthorizedError} = require('../../../../core/server/lib/common/errors'); const sessionController = require('../../../../core/server/api/canary/session'); -const sessionServiceMiddleware = require('../../../../core/server/services/auth/session').middleware; +const sessionServiceMiddleware = require('../../../../core/server/services/auth/session'); describe('v3 Session controller', function () { before(function () { diff --git a/test/unit/services/auth/session/middleware_spec.js b/test/unit/services/auth/session/middleware_spec.js index 1f0be4fb5ab6..47eae7ec542b 100644 --- a/test/unit/services/auth/session/middleware_spec.js +++ b/test/unit/services/auth/session/middleware_spec.js @@ -2,11 +2,6 @@ const sessionMiddleware = require('../../../../../core/server/services/auth').se const models = require('../../../../../core/server/models'); const sinon = require('sinon'); const should = require('should'); -const { - BadRequestError, - UnauthorizedError, - InternalServerError -} = require('../../../../../core/server/lib/common/errors'); describe('Session Service', function () { before(function () { @@ -22,6 +17,7 @@ describe('Session Service', function () { session: { destroy() {} }, + user: null, body: {}, get() {} }; @@ -34,18 +30,6 @@ describe('Session Service', function () { }; describe('createSession', function () { - it('calls next with a BadRequestError if there is no Origin or Refferer', function (done) { - const req = fakeReq(); - sinon.stub(req, 'get') - .withArgs('origin').returns('') - .withArgs('referrer').returns(''); - - sessionMiddleware.createSession(req, fakeRes(), function next(err) { - should.equal(err instanceof BadRequestError, true); - done(); - }); - }); - it('sets req.session.origin from the Referer header', function (done) { const req = fakeReq(); const res = fakeRes(); @@ -59,7 +43,7 @@ describe('Session Service', function () { req.user = models.User.forge({id: 23}); sinon.stub(res, 'sendStatus') - .callsFake(function (statusCode) { + .callsFake(function () { should.equal(req.session.origin, 'http://ghost.org'); done(); }); @@ -102,7 +86,7 @@ describe('Session Service', function () { }); sinon.stub(res, 'sendStatus') - .callsFake(function (statusCode) { + .callsFake(function () { should.equal(destroyStub.callCount, 1); done(); }); @@ -119,7 +103,7 @@ describe('Session Service', function () { }); sessionMiddleware.destroySession(req, res, function next(err) { - should.equal(err instanceof InternalServerError, true); + should.equal(err.errorType, 'InternalServerError'); done(); }); }); diff --git a/yarn.lock b/yarn.lock index 2bc19e341231..a54ce55d7c4d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -435,6 +435,11 @@ mobiledoc-dom-renderer "0.7.0" mobiledoc-text-renderer "0.4.0" +"@tryghost/mw-session-from-token@^0.1.0": + version "0.1.0" + resolved "https://registry.yarnpkg.com/@tryghost/mw-session-from-token/-/mw-session-from-token-0.1.0.tgz#ad73da936d3cc3abdfc7753a5098e76ae5dc271e" + integrity sha512-t6a9YMMHaQ2Uypl+WaGLc4mFKjwhfyfA+l7V9H5DwFeAFIYWcRssy/D3e+gTT3rbLC39W0vqkmZFNE2JQZuuEQ== + "@tryghost/pretty-cli@1.2.3": version "1.2.3" resolved "https://registry.yarnpkg.com/@tryghost/pretty-cli/-/pretty-cli-1.2.3.tgz#06fc84e4659ffde7166ca2a9ebe17c3951438a6c"