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

@@ -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)
       })
     })

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',
   },
 }

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) {
+    it('should return a lookup error', function (done) {
-      return this.checkSocket(error => {
+      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) {
+    it('should return a lookup error', function (done) {
-      return this.checkSocket(error => {
+      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) {
+    it('should return a lookup error', function (done) {
-      return this.checkSocket(error => {
+      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) {
+    it('should return a lookup error', function (done) {
-      return this.checkSocket(error => {
+      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 () {
+  describe('with a valid cookie and a matching session', function () {
-    return it('should not return an error', function (done) {
+    it('should not return an error', function (done) {
-      return this.checkSocket(error => {
+      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()
       })
     })
   })

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
+      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.cookie = `${Settings.cookieName}=${cookieKey}`
+        Client.cookieSignedWith[secretName] =
+          `${Settings.cookieName}=${cookieKey}`
+      }
+      // default to the current session secret
+      Client.cookie = Client.cookieSignedWith.sessionSecret
       return callback()
     })
   },

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 () {
+  beforeEach(function () {
-    this.SessionSocketsModule = SandboxedModule.require(modulePath)
+    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 () {
+  describe('with a different valid cookie and matching session', function () {
-    before(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()
+      })
+    })
   })
 })