From 29105911c509f7f2fe2ff9f5d6b81ba106dd91a2 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 25 Apr 2024 16:32:01 +0100 Subject: [PATCH] Merge pull request #17732 from overleaf/bg-session-mitigation-initial-protoype anonymous cookie-based sessions module GitOrigin-RevId: 75fe2d48fa384ba8d07c0b478a9a5a907a2b3b67 --- package-lock.json | 4 +++ .../AuthenticationController.js | 5 ++-- .../web/app/src/infrastructure/Modules.js | 13 ++++++--- services/web/app/src/infrastructure/Server.js | 5 +++- .../src/infrastructure/SessionStoreManager.js | 3 +++ services/web/package.json | 2 ++ .../test/acceptance/src/helpers/InitApp.js | 26 ++++++++++++++++++ .../web/test/acceptance/src/helpers/User.js | 27 +++++++++++++++++-- .../AuthenticationControllerTests.js | 4 +-- 9 files changed, 78 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index ee85701f1f..af5c2a843a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -43377,6 +43377,7 @@ "node-fetch": "^2.6.7", "nodemailer": "^6.7.0", "nodemailer-ses-transport": "^1.5.1", + "on-headers": "^1.0.2", "openai": "^4.36.0", "otplib": "^12.0.1", "p-limit": "^2.3.0", @@ -43398,6 +43399,7 @@ "sanitize-html": "^2.8.1", "tough-cookie": "^4.0.0", "tsscmp": "^1.0.6", + "uid-safe": "^2.1.5", "utf-8-validate": "^5.0.2", "uuid": "^9.0.1", "valid-data-url": "^2.0.0", @@ -51977,6 +51979,7 @@ "nodemailer": "^6.7.0", "nodemailer-ses-transport": "^1.5.1", "nvd3": "^1.8.6", + "on-headers": "^1.0.2", "openai": "^4.36.0", "otplib": "^12.0.1", "p-limit": "^2.3.0", @@ -52035,6 +52038,7 @@ "tough-cookie": "^4.0.0", "tsscmp": "^1.0.6", "typescript": "^5.0.4", + "uid-safe": "^2.1.5", "utf-8-validate": "^5.0.2", "uuid": "^9.0.1", "valid-data-url": "^2.0.0", diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 84bc2640d2..a9446718bc 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -419,8 +419,9 @@ const AuthenticationController = { return function (req, res, next) { // check that the session store is returning valid results if (req.session && !SessionStoreManager.hasValidationToken(req)) { - // force user to update session - req.session.regenerate(() => { + // Force user to update session by destroying the current one. + // A new session will be created on the next request. + req.session.destroy(() => { // need to destroy the existing session and generate a new one // otherwise they will already be logged in when they are redirected // to the login page diff --git a/services/web/app/src/infrastructure/Modules.js b/services/web/app/src/infrastructure/Modules.js index cabfbefb10..54fe261256 100644 --- a/services/web/app/src/infrastructure/Modules.js +++ b/services/web/app/src/infrastructure/Modules.js @@ -88,10 +88,15 @@ function loadViewIncludes(app) { } } -function registerAppMiddleware(app) { +function registerMiddleware(appOrRouter, middlewareName, options) { + if (!middlewareName) { + throw new Error( + 'middleware name must be provided to register module middleware' + ) + } for (const module of modules()) { - if (module.appMiddleware) { - module.appMiddleware(app) + if (module[middlewareName]) { + module[middlewareName](appOrRouter, options) } } } @@ -164,7 +169,7 @@ module.exports = { loadViewIncludes, moduleIncludes, moduleIncludesAvailable, - registerAppMiddleware, + registerMiddleware, hooks: { attach: attachHook, fire: fireHook, diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index a20c14f142..067db600a6 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -133,7 +133,7 @@ Modules.loadViewIncludes(app) app.use(metrics.http.monitor(logger)) -Modules.registerAppMiddleware(app) +Modules.registerMiddleware(app, 'appMiddleware') app.use(bodyParser.urlencoded({ extended: true, limit: '2mb' })) app.use(bodyParser.json({ limit: Settings.max_json_request_size })) app.use(methodOverride()) @@ -157,6 +157,9 @@ RedirectManager.apply(webRouter) webRouter.use(cookieParser(Settings.security.sessionSecret)) SessionAutostartMiddleware.applyInitialMiddleware(webRouter) +Modules.registerMiddleware(webRouter, 'sessionMiddleware', { + store: sessionStore, +}) webRouter.use( session({ resave: false, diff --git a/services/web/app/src/infrastructure/SessionStoreManager.js b/services/web/app/src/infrastructure/SessionStoreManager.js index 10502ff1b2..168c0091a9 100644 --- a/services/web/app/src/infrastructure/SessionStoreManager.js +++ b/services/web/app/src/infrastructure/SessionStoreManager.js @@ -71,4 +71,7 @@ module.exports = { return false } }, + + computeValidationToken, + checkValidationToken, } diff --git a/services/web/package.json b/services/web/package.json index e6b2a70f08..d5ce623e23 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -136,6 +136,7 @@ "node-fetch": "^2.6.7", "nodemailer": "^6.7.0", "nodemailer-ses-transport": "^1.5.1", + "on-headers": "^1.0.2", "openai": "^4.36.0", "otplib": "^12.0.1", "p-limit": "^2.3.0", @@ -157,6 +158,7 @@ "sanitize-html": "^2.8.1", "tough-cookie": "^4.0.0", "tsscmp": "^1.0.6", + "uid-safe": "^2.1.5", "utf-8-validate": "^5.0.2", "uuid": "^9.0.1", "valid-data-url": "^2.0.0", diff --git a/services/web/test/acceptance/src/helpers/InitApp.js b/services/web/test/acceptance/src/helpers/InitApp.js index 56c88758f2..4feb103f5a 100644 --- a/services/web/test/acceptance/src/helpers/InitApp.js +++ b/services/web/test/acceptance/src/helpers/InitApp.js @@ -28,6 +28,11 @@ before('start main app', function (done) { route => route.path && route.path === '/dev/csrf', router => { router.get('/dev/session', (req, res) => { + // allow changing the session directly for testing, assign any + // properties in the query string to req.session + if (req.query && Object.keys(req.query).length > 0) { + Object.assign(req.session, req.query) + } return res.json(req.session) }) } @@ -75,6 +80,27 @@ before('start main app', function (done) { }) } ) + injectRouteAfter( + app, + route => route.path && route.path === '/dev/csrf', + router => { + router.csrf.disableDefaultCsrfProtection( + '/dev/no_autostart_post_gateway', + 'POST' + ) + router.sessionAutostartMiddleware.disableSessionAutostartForRoute( + '/dev/no_autostart_post_gateway', + 'POST', + (req, res, next) => { + next() + } + ) + router.post('/dev/no_autostart_post_gateway', (req, res) => { + res.status(200).json({ message: 'no autostart' }) + }) + } + ) + server = App.listen(23000, '127.0.0.1', done) }) diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 011ba1f4de..cde2f7807d 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -8,6 +8,8 @@ const AuthenticationManager = require('../../../../app/src/Features/Authenticati const { promisifyClass } = require('@overleaf/promise-utils') const fs = require('fs') const Path = require('path') +const { Cookie } = require('tough-cookie') +const COOKIE_DOMAIN = settings.cookieDomain let count = settings.test.counterInit @@ -32,10 +34,15 @@ class User { }) } - getSession(callback) { + getSession(options, callback) { + if (typeof options === 'function') { + callback = options + options = {} + } this.request.get( { url: '/dev/session', + qs: options.set, }, (err, response, body) => { if (err != null) { @@ -186,6 +193,22 @@ class User { ) } + /* Return the session cookie, url decoded. Use the option {raw:true} to get the original undecoded value */ + + sessionCookie(options) { + const cookie = Cookie.parse( + this.jar.getCookieString( + // The cookie domain has a leading '.' but + // the cookie jar stores it without. + 'https://' + COOKIE_DOMAIN.replace(/^\./, '') + '/' + ) + ) + if (cookie?.value && !options?.raw) { + cookie.value = decodeURIComponent(cookie.value) + } + return cookie + } + getEmailConfirmationCode(callback) { this.getSession((err, session) => { if (err != null) { @@ -1224,7 +1247,7 @@ class User { } User.promises = promisifyClass(User, { - without: ['setExtraAttributes'], + without: ['setExtraAttributes', 'sessionCookie'], }) User.promises.prototype.doRequest = async function (method, params) { diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 896ca24442..0d47c30f1f 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -624,7 +624,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.req.session = { user: this.user, - regenerate: sinon.stub().yields(), + destroy: sinon.stub().yields(), } this.req.user = this.user this.AuthenticationController._redirectToLoginOrRegisterPage = @@ -637,7 +637,7 @@ describe('AuthenticationController', function () { }) it('should destroy the current session', function () { - this.req.session.regenerate.called.should.equal(true) + this.req.session.destroy.called.should.equal(true) }) it('should redirect to the register or login page', function () {