From 369d5cb4064658ebe93ca093f69b1a4908399673 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Mon, 11 Dec 2023 14:13:31 +0100 Subject: [PATCH] Merge pull request #16190 from overleaf/revert-15519-em-upgrade-passport Revert "Upgrade passport" GitOrigin-RevId: 34a5442d6dae9623463908f92ab103bdc16f1b67 --- package-lock.json | 26 ++--- .../AuthenticationController.js | 99 +++++++++++-------- .../app/src/Features/User/UserController.js | 8 +- services/web/package.json | 2 +- .../AuthenticationControllerTests.js | 43 ++++---- .../test/unit/src/User/UserControllerTests.js | 2 +- 6 files changed, 95 insertions(+), 85 deletions(-) diff --git a/package-lock.json b/package-lock.json index 23411d2656..b671bbbdad 100644 --- a/package-lock.json +++ b/package-lock.json @@ -30573,20 +30573,15 @@ } }, "node_modules/passport": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz", - "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==", + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz", + "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==", "dependencies": { "passport-strategy": "1.x.x", - "pause": "0.0.1", - "utils-merge": "^1.0.1" + "pause": "0.0.1" }, "engines": { "node": ">= 0.4.0" - }, - "funding": { - "type": "github", - "url": "https://github.com/sponsors/jaredhanson" } }, "node_modules/passport-google-oauth20": { @@ -43530,7 +43525,7 @@ "otplib": "^12.0.1", "p-limit": "^2.3.0", "parse-data-url": "^2.0.0", - "passport": "^0.6.0", + "passport": "^0.4.1", "passport-google-oauth20": "^2.0.0", "passport-ldapauth": "^2.1.4", "passport-local": "^1.0.0", @@ -52272,7 +52267,7 @@ "otplib": "^12.0.1", "p-limit": "^2.3.0", "parse-data-url": "^2.0.0", - "passport": "^0.6.0", + "passport": "^0.4.1", "passport-google-oauth20": "^2.0.0", "passport-ldapauth": "^2.1.4", "passport-local": "^1.0.0", @@ -71002,13 +70997,12 @@ } }, "passport": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz", - "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==", + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz", + "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==", "requires": { "passport-strategy": "1.x.x", - "pause": "0.0.1", - "utils-merge": "^1.0.1" + "pause": "0.0.1" } }, "passport-google-oauth20": { diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 56db9e8f89..3ad4709032 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -80,36 +80,30 @@ const AuthenticationController = { // This function is middleware which wraps the passport.authenticate middleware, // so we can send back our custom `{message: {text: "", type: ""}}` responses on failure, // and send a `{redir: ""}` response on success - passport.authenticate( - 'local', - { keepSessionInfo: true }, - function (err, user, info) { - if (err) { - return next(err) - } - if (user) { - // `user` is either a user object or false - AuthenticationController.setAuditInfo(req, { - method: 'Password login', - }) - return AuthenticationController.finishLogin(user, req, res, next) + passport.authenticate('local', function (err, user, info) { + if (err) { + return next(err) + } + if (user) { + // `user` is either a user object or false + AuthenticationController.setAuditInfo(req, { method: 'Password login' }) + return AuthenticationController.finishLogin(user, req, res, next) + } else { + if (info.redir != null) { + return res.json({ redir: info.redir }) } else { - if (info.redir != null) { - return res.json({ redir: info.redir }) - } else { - res.status(info.status || 200) - delete info.status - const body = { message: info } - const { errorReason } = info - if (errorReason) { - body.errorReason = errorReason - delete info.errorReason - } - return res.json(body) + res.status(info.status || 200) + delete info.status + const body = { message: info } + const { errorReason } = info + if (errorReason) { + body.errorReason = errorReason + delete info.errorReason } + return res.json(body) } } - )(req, res, next) + })(req, res, next) }, finishLogin(user, req, res, next) { @@ -563,34 +557,55 @@ const AuthenticationController = { } function _afterLoginSessionSetup(req, user, callback) { - req.login(user, { keepSessionInfo: true }, function (err) { + if (callback == null) { + callback = function () {} + } + req.login(user, function (err) { if (err) { OError.tag(err, 'error from req.login', { user_id: user._id, }) return callback(err) } - delete req.session.__tmp - delete req.session.csrfSecret - req.session.save(function (err) { + // Regenerate the session to get a new sessionID (cookie value) to + // protect against session fixation attacks + const oldSession = req.session + req.session.destroy(function (err) { if (err) { - OError.tag(err, 'error saving regenerated session after login', { + OError.tag(err, 'error when trying to destroy old session', { user_id: user._id, }) return callback(err) } - UserSessionsManager.trackSession(user, req.sessionID, function () {}) - if (!req.deviceHistory) { - // Captcha disabled or SSO-based login. - return callback() + req.sessionStore.generate(req) + // Note: the validation token is not writable, so it does not get + // transferred to the new session below. + for (const key in oldSession) { + const value = oldSession[key] + if (key !== '__tmp' && key !== 'csrfSecret') { + req.session[key] = value + } } - req.deviceHistory.add(user.email) - req.deviceHistory - .serialize(req.res) - .catch(err => { - logger.err({ err }, 'cannot serialize deviceHistory') - }) - .finally(() => callback()) + req.session.save(function (err) { + if (err) { + OError.tag(err, 'error saving regenerated session after login', { + user_id: user._id, + }) + return callback(err) + } + UserSessionsManager.trackSession(user, req.sessionID, function () {}) + if (!req.deviceHistory) { + // Captcha disabled or SSO-based login. + return callback() + } + req.deviceHistory.add(user.email) + req.deviceHistory + .serialize(req.res) + .catch(err => { + logger.err({ err }, 'cannot serialize deviceHistory') + }) + .finally(() => callback()) + }) }) }) } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index d557c85c84..f2a6070f50 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -266,8 +266,7 @@ async function tryDeleteUser(req, res, next) { const sessionId = req.sessionID if (typeof req.logout === 'function') { - const logout = promisify(req.logout) - await logout() + req.logout() } const destroySession = promisify(req.session.destroy.bind(req.session)) @@ -432,10 +431,9 @@ async function doLogout(req) { logger.debug({ user }, 'logging out') const sessionId = req.sessionID + // passport logout if (typeof req.logout === 'function') { - // passport logout - const logout = promisify(req.logout.bind(req)) - await logout() + req.logout() } const destroySession = promisify(req.session.destroy.bind(req.session)) diff --git a/services/web/package.json b/services/web/package.json index 68140cce7b..1777a9f5e8 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -137,7 +137,7 @@ "otplib": "^12.0.1", "p-limit": "^2.3.0", "parse-data-url": "^2.0.0", - "passport": "^0.6.0", + "passport": "^0.4.1", "passport-google-oauth20": "^2.0.0", "passport-ldapauth": "^2.1.4", "passport-local": "^1.0.0", diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index cc0b08cbf5..15bd16199f 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -77,7 +77,7 @@ describe('AuthenticationController', function () { '../User/UserSessionsManager': (this.UserSessionsManager = { trackSession: sinon.stub(), untrackSession: sinon.stub(), - revokeAllUserSessions: sinon.stub().yields(null), + revokeAllUserSessions: sinon.stub().callsArgWith(1, null), }), '../../infrastructure/Modules': (this.Modules = { hooks: { fire: sinon.stub().yields(null, []) }, @@ -184,17 +184,17 @@ describe('AuthenticationController', function () { describe('passportLogin', function () { beforeEach(function () { this.info = null - this.req.login = sinon.stub().yields(null) + this.req.login = sinon.stub().callsArgWith(1, null) this.res.json = sinon.stub() this.req.session = { passport: { user: this.user }, postLoginRedirect: '/path/to/redir/to', } - this.req.session.destroy = sinon.stub().yields(null) - this.req.session.save = sinon.stub().yields(null) + this.req.session.destroy = sinon.stub().callsArgWith(0, null) + this.req.session.save = sinon.stub().callsArgWith(0, null) this.req.sessionStore = { generate: sinon.stub() } this.AuthenticationController.finishLogin = sinon.stub() - this.passport.authenticate.yields(null, this.user, this.info) + this.passport.authenticate.callsArgWith(1, null, this.user, this.info) this.err = new Error('woops') }) @@ -205,7 +205,7 @@ describe('AuthenticationController', function () { describe('when authenticate produces an error', function () { beforeEach(function () { - this.passport.authenticate.yields(this.err) + this.passport.authenticate.callsArgWith(1, this.err) }) it('should return next with an error', function () { @@ -221,7 +221,7 @@ describe('AuthenticationController', function () { describe('when authenticate produces a user', function () { beforeEach(function () { this.req.session.postLoginRedirect = 'some_redirect' - this.passport.authenticate.yields(null, this.user, this.info) + this.passport.authenticate.callsArgWith(1, null, this.user, this.info) }) afterEach(function () { @@ -244,7 +244,7 @@ describe('AuthenticationController', function () { describe('when authenticate does not produce a user', function () { beforeEach(function () { this.info = { text: 'a', type: 'b' } - this.passport.authenticate.yields(null, false, this.info) + this.passport.authenticate.callsArgWith(1, null, false, this.info) }) it('should not call finishLogin', function () { @@ -273,7 +273,8 @@ describe('AuthenticationController', function () { beforeEach(function () { this.AuthenticationController._recordFailedLogin = sinon.stub() this.AuthenticationController._recordSuccessfulLogin = sinon.stub() - this.Modules.hooks.fire = sinon.stub().yields(null, []) + this.Modules.hooks.fire = sinon.stub().callsArgWith(3, null, []) + // @AuthenticationController.establishUserSession = sinon.stub().callsArg(2) this.req.body = { email: this.email, password: this.password, @@ -289,7 +290,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.Modules.hooks.fire = sinon .stub() - .yields(null, [null, { redir: '/somewhere' }, null]) + .callsArgWith(3, null, [null, { redir: '/somewhere' }, null]) }) it('should stop early and call done with this info object', function (done) { @@ -310,7 +311,7 @@ describe('AuthenticationController', function () { describe('when the users rate limit', function () { beforeEach(function () { - this.LoginRateLimiter.processLoginRequest.yields(null, false) + this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, false) }) it('should block the request if the limit has been exceeded', function (done) { @@ -329,10 +330,10 @@ describe('AuthenticationController', function () { describe('when the user is authenticated', function () { beforeEach(function () { this.cb = sinon.stub() - this.LoginRateLimiter.processLoginRequest.yields(null, true) + this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) this.AuthenticationManager.authenticate = sinon .stub() - .yields(null, this.user) + .callsArgWith(3, null, this.user) this.req.sessionID = Math.random() }) @@ -360,7 +361,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.AuthenticationManager.authenticate = sinon .stub() - .yields(new AuthenticationErrors.ParallelLoginError()) + .callsArgWith(3, new AuthenticationErrors.ParallelLoginError()) this.AuthenticationController.doPassportLogin( this.req, this.req.body.email, @@ -436,10 +437,10 @@ describe('AuthenticationController', function () { describe('when the user is not authenticated', function () { beforeEach(function () { - this.LoginRateLimiter.processLoginRequest.yields(null, true) + this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) this.AuthenticationManager.authenticate = sinon .stub() - .yields(null, null) + .callsArgWith(3, null, null) this.cb = sinon.stub() this.AuthenticationController.doPassportLogin( this.req, @@ -936,7 +937,7 @@ describe('AuthenticationController', function () { describe('_recordSuccessfulLogin', function () { beforeEach(function () { - this.UserUpdater.updateUser = sinon.stub().yields() + this.UserUpdater.updateUser = sinon.stub().callsArg(2) this.AuthenticationController._recordSuccessfulLogin( this.user._id, this.callback @@ -1077,8 +1078,8 @@ describe('AuthenticationController', function () { this.req.session = { passport: { user: { _id: 'one' } }, } - this.req.session.destroy = sinon.stub().yields(null) - this.req.session.save = sinon.stub().yields(null) + this.req.session.destroy = sinon.stub().callsArgWith(0, null) + this.req.session.save = sinon.stub().callsArgWith(0, null) this.req.sessionStore = { generate: sinon.stub() } this.req.login = sinon.stub().yields(null) @@ -1344,7 +1345,9 @@ describe('AuthenticationController', function () { describe('when req.session.save produces an error', function () { beforeEach(function () { - this.req.session.save = sinon.stub().yields(new Error('woops')) + this.req.session.save = sinon + .stub() + .callsArgWith(0, new Error('woops')) }) it('should produce an error', function (done) { diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 8409163aa1..d6b4f86333 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -173,7 +173,7 @@ describe('UserController', function () { describe('tryDeleteUser', function () { beforeEach(function () { this.req.body.password = 'wat' - this.req.logout = sinon.stub().yields() + this.req.logout = sinon.stub() this.req.session.destroy = sinon.stub().yields() this.SessionManager.getLoggedInUserId = sinon .stub()