From 49dc94192aef0c1a6195ff18e1327651aa19e1f6 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 12 Jun 2024 10:51:37 +0100 Subject: [PATCH] Merge pull request #18848 from overleaf/bg-cookie-session-metrics-for-real-time add cookie session metrics for real-time GitOrigin-RevId: 6b482dafc19fca46a375ea00a9b2297e20d915ce --- services/real-time/app/js/SessionSockets.js | 9 ++ services/real-time/config/settings.test.js | 1 + .../test/acceptance/js/SessionSocketsTests.js | 96 ++++++++++----- .../acceptance/js/helpers/RealTimeClient.js | 17 ++- .../test/unit/js/SessionSocketsTests.js | 112 +++++++++++++++--- 5 files changed, 186 insertions(+), 49 deletions(-) diff --git a/services/real-time/app/js/SessionSockets.js b/services/real-time/app/js/SessionSockets.js index b8ccde2ea8..ddec2056bc 100644 --- a/services/real-time/app/js/SessionSockets.js +++ b/services/real-time/app/js/SessionSockets.js @@ -1,3 +1,4 @@ +const metrics = require('@overleaf/metrics') const OError = require('@overleaf/o-error') const { EventEmitter } = require('events') const { MissingSessionError } = require('./Errors') @@ -15,18 +16,26 @@ module.exports = function (io, sessionStore, cookieParser, cookieName) { cookieParser(req, {}, function () { const sessionId = req.signedCookies && req.signedCookies[cookieName] if (!sessionId) { + metrics.inc('session.cookie', 1, { + // the cookie-parser middleware sets the signed cookie to false if the + // signature is invalid, so we can use this to detect bad signatures + status: sessionId === false ? 'bad-signature' : 'none', + }) return next(missingSessionError, socket) } sessionStore.get(sessionId, function (error, session) { if (error) { + metrics.inc('session.cookie', 1, { status: 'error' }) OError.tag(error, 'error getting session from sessionStore', { sessionId, }) return next(error, socket) } if (!session) { + metrics.inc('session.cookie', 1, { status: 'missing' }) return next(missingSessionError, socket) } + metrics.inc('session.cookie', 1, { status: 'signed' }) next(null, socket, session) }) }) diff --git a/services/real-time/config/settings.test.js b/services/real-time/config/settings.test.js index cb02982be8..e74a6fbe41 100644 --- a/services/real-time/config/settings.test.js +++ b/services/real-time/config/settings.test.js @@ -6,5 +6,6 @@ module.exports = { security: { sessionSecret: 'static-secret-for-tests', sessionSecretFallback: 'static-secret-fallback-for-tests', + sessionSecretUpcoming: 'static-secret-upcoming-for-tests', }, } diff --git a/services/real-time/test/acceptance/js/SessionSocketsTests.js b/services/real-time/test/acceptance/js/SessionSocketsTests.js index 93dd08d2de..cca7f75d07 100644 --- a/services/real-time/test/acceptance/js/SessionSocketsTests.js +++ b/services/real-time/test/acceptance/js/SessionSocketsTests.js @@ -1,16 +1,7 @@ -/* eslint-disable - no-return-assign, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const RealTimeClient = require('./helpers/RealTimeClient') const FixturesManager = require('./helpers/FixturesManager') const Settings = require('@overleaf/settings') +const signature = require('cookie-signature') const { expect } = require('chai') describe('SessionSockets', function () { @@ -32,28 +23,28 @@ describe('SessionSockets', function () { describe('without cookies', function () { beforeEach(function () { - return (RealTimeClient.cookie = null) + RealTimeClient.cookie = null }) - return it('should return a lookup error', function (done) { - return this.checkSocket(error => { + it('should return a lookup error', function (done) { + this.checkSocket(error => { expect(error).to.exist expect(error.message).to.equal('invalid session') - return done() + done() }) }) }) describe('with a different cookie', function () { beforeEach(function () { - return (RealTimeClient.cookie = 'some.key=someValue') + RealTimeClient.cookie = 'some.key=someValue' }) - return it('should return a lookup error', function (done) { - return this.checkSocket(error => { + it('should return a lookup error', function (done) { + this.checkSocket(error => { expect(error).to.exist expect(error.message).to.equal('invalid session') - return done() + done() }) }) }) @@ -67,39 +58,82 @@ describe('SessionSockets', function () { RealTimeClient.cookie = `${ Settings.cookieName }=${RealTimeClient.cookie.slice(17, 49)}` - return done() + done() }) - return null }) - return it('should return a lookup error', function (done) { - return this.checkSocket(error => { + it('should return a lookup error', function (done) { + this.checkSocket(error => { expect(error).to.exist expect(error.message).to.equal('invalid session') - return done() + done() }) }) }) describe('with a valid cookie and no matching session', function () { beforeEach(function () { - return (RealTimeClient.cookie = `${Settings.cookieName}=unknownId`) + RealTimeClient.cookie = `${Settings.cookieName}=unknownId` }) - return it('should return a lookup error', function (done) { - return this.checkSocket(error => { + it('should return a lookup error', function (done) { + this.checkSocket(error => { expect(error).to.exist expect(error.message).to.equal('invalid session') - return done() + done() }) }) }) - return describe('with a valid cookie and a matching session', function () { - return it('should not return an error', function (done) { - return this.checkSocket(error => { + describe('with a valid cookie and a matching session', function () { + it('should not return an error', function (done) { + this.checkSocket(error => { expect(error).to.not.exist - return done() + done() + }) + }) + }) + + describe('with a cookie signed by the fallback key and a matching session', function () { + beforeEach(function () { + RealTimeClient.cookie = + RealTimeClient.cookieSignedWith.sessionSecretFallback + }) + it('should not return an error', function (done) { + this.checkSocket(error => { + expect(error).to.not.exist + done() + }) + }) + }) + + describe('with a cookie signed by the upcoming key and a matching session', function () { + beforeEach(function () { + RealTimeClient.cookie = + RealTimeClient.cookieSignedWith.sessionSecretUpcoming + }) + it('should not return an error', function (done) { + this.checkSocket(error => { + expect(error).to.not.exist + done() + }) + }) + }) + + describe('with a cookie signed with an unrecognized secret and a matching session', function () { + beforeEach(function () { + const [sessionKey] = RealTimeClient.cookie.split('.') + // sign the session key with a unrecognized secret + RealTimeClient.cookie = signature.sign( + sessionKey, + 'unrecognised-session-secret' + ) + }) + it('should return a lookup error', function (done) { + this.checkSocket(error => { + expect(error).to.exist + expect(error.message).to.equal('invalid session') + done() }) }) }) diff --git a/services/real-time/test/acceptance/js/helpers/RealTimeClient.js b/services/real-time/test/acceptance/js/helpers/RealTimeClient.js index c278787414..7b53f5d5c4 100644 --- a/services/real-time/test/acceptance/js/helpers/RealTimeClient.js +++ b/services/real-time/test/acceptance/js/helpers/RealTimeClient.js @@ -47,9 +47,20 @@ module.exports = Client = { if (error != null) { return callback(error) } - const secret = Settings.security.sessionSecret - const cookieKey = 's:' + signature.sign(sessionId, secret) - Client.cookie = `${Settings.cookieName}=${cookieKey}` + Client.cookieSignedWith = {} + // prepare cookie strings for all supported session secrets + for (const secretName of [ + 'sessionSecret', + 'sessionSecretFallback', + 'sessionSecretUpcoming', + ]) { + const secret = Settings.security[secretName] + const cookieKey = 's:' + signature.sign(sessionId, secret) + Client.cookieSignedWith[secretName] = + `${Settings.cookieName}=${cookieKey}` + } + // default to the current session secret + Client.cookie = Client.cookieSignedWith.sessionSecret return callback() }) }, diff --git a/services/real-time/test/unit/js/SessionSocketsTests.js b/services/real-time/test/unit/js/SessionSocketsTests.js index 3a3eb604ef..e296ca00d3 100644 --- a/services/real-time/test/unit/js/SessionSocketsTests.js +++ b/services/real-time/test/unit/js/SessionSocketsTests.js @@ -15,8 +15,13 @@ const modulePath = '../../../app/js/SessionSockets' const sinon = require('sinon') describe('SessionSockets', function () { - before(function () { - this.SessionSocketsModule = SandboxedModule.require(modulePath) + beforeEach(function () { + this.metrics = { inc: sinon.stub() } + this.SessionSocketsModule = SandboxedModule.require(modulePath, { + requires: { + '@overleaf/metrics': this.metrics, + }, + }) this.io = new EventEmitter() this.id1 = Math.random().toString() this.id2 = Math.random().toString() @@ -49,7 +54,7 @@ describe('SessionSockets', function () { }) describe('without cookies', function () { - before(function () { + beforeEach(function () { return (this.socket = { handshake: {} }) }) @@ -61,16 +66,25 @@ describe('SessionSockets', function () { }) }) - return it('should not query redis', function (done) { + it('should not query redis', function (done) { return this.checkSocket(this.socket, () => { expect(this.sessionStore.get.called).to.equal(false) return done() }) }) + + it('should increment the session.cookie metric with status "none"', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.metrics.inc).to.be.calledWith('session.cookie', 1, { + status: 'none', + }) + return done() + }) + }) }) describe('with a different cookie', function () { - before(function () { + beforeEach(function () { return (this.socket = { handshake: { _signedCookies: { other: 1 } } }) }) @@ -82,7 +96,7 @@ describe('SessionSockets', function () { }) }) - return it('should not query redis', function (done) { + it('should not query redis', function (done) { return this.checkSocket(this.socket, () => { expect(this.sessionStore.get.called).to.equal(false) return done() @@ -90,8 +104,40 @@ describe('SessionSockets', function () { }) }) + describe('with a cookie with an invalid signature', function () { + beforeEach(function () { + return (this.socket = { + handshake: { _signedCookies: { 'ol.sid': false } }, + }) + }) + + it('should return a lookup error', function (done) { + return this.checkSocket(this.socket, error => { + expect(error).to.exist + expect(error.message).to.equal('could not look up session by key') + return done() + }) + }) + + it('should not query redis', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.sessionStore.get.called).to.equal(false) + return done() + }) + }) + + it('should increment the session.cookie metric with status=bad-signature', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.metrics.inc).to.be.calledWith('session.cookie', 1, { + status: 'bad-signature', + }) + return done() + }) + }) + }) + describe('with a valid cookie and a failing session lookup', function () { - before(function () { + beforeEach(function () { return (this.socket = { handshake: { _signedCookies: { 'ol.sid': 'error' } }, }) @@ -104,17 +150,26 @@ describe('SessionSockets', function () { }) }) - return it('should return a redis error', function (done) { + it('should return a redis error', function (done) { return this.checkSocket(this.socket, error => { expect(error).to.exist expect(error.message).to.equal('Redis: something went wrong') return done() }) }) + + it('should increment the session.cookie metric with status=error', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.metrics.inc).to.be.calledWith('session.cookie', 1, { + status: 'error', + }) + return done() + }) + }) }) describe('with a valid cookie and no matching session', function () { - before(function () { + beforeEach(function () { return (this.socket = { handshake: { _signedCookies: { 'ol.sid': 'unknownId' } }, }) @@ -127,17 +182,26 @@ describe('SessionSockets', function () { }) }) - return it('should return a lookup error', function (done) { + it('should return a lookup error', function (done) { return this.checkSocket(this.socket, error => { expect(error).to.exist expect(error.message).to.equal('could not look up session by key') return done() }) }) + + it('should increment the session.cookie metric with status=missing', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.metrics.inc).to.be.calledWith('session.cookie', 1, { + status: 'missing', + }) + return done() + }) + }) }) describe('with a valid cookie and a matching session', function () { - before(function () { + beforeEach(function () { return (this.socket = { handshake: { _signedCookies: { 'ol.sid': this.id1 } }, }) @@ -157,17 +221,26 @@ describe('SessionSockets', function () { }) }) - return it('should return the session', function (done) { + it('should return the session', function (done) { return this.checkSocket(this.socket, (error, s, session) => { if (error) return done(error) expect(session).to.deep.equal({ user: { _id: '123' } }) return done() }) }) + + it('should increment the session.cookie metric with status=signed', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.metrics.inc).to.be.calledWith('session.cookie', 1, { + status: 'signed', + }) + return done() + }) + }) }) - return describe('with a different valid cookie and matching session', function () { - before(function () { + describe('with a different valid cookie and matching session', function () { + beforeEach(function () { return (this.socket = { handshake: { _signedCookies: { 'ol.sid': this.id2 } }, }) @@ -187,12 +260,21 @@ describe('SessionSockets', function () { }) }) - return it('should return the other session', function (done) { + it('should return the other session', function (done) { return this.checkSocket(this.socket, (error, s, session) => { if (error) return done(error) expect(session).to.deep.equal({ user: { _id: 'abc' } }) return done() }) }) + + it('should increment the session.cookie metric with status=error', function (done) { + return this.checkSocket(this.socket, () => { + expect(this.metrics.inc).to.be.calledWith('session.cookie', 1, { + status: 'signed', + }) + return done() + }) + }) }) })