Merge pull request #18775 from overleaf/bg-cookie-session-metrics-middleware

add middleware to record session cookie metrics in web

GitOrigin-RevId: f4404455e219d2071d6f0b39e657e9219b7d1c70

services/web/app/src/infrastructure/CookieMetrics.js

@@ -0,0 +1,30 @@
+const Settings = require('@overleaf/settings')
+const metrics = require('@overleaf/metrics')
+
+/**
+ * Middleware function to record session cookie metrics.  This allows us to
+ * detect whether users are sending valid signed cookies, cookies with invalid
+ * signatures (e.g. using an old key), or no cookies at all.
+ *
+ * Signed cookies begin with the prefix 's:'.  If the signature fails to verify,
+ * the signed cookie value is returned as false.
+ */
+function middleware(req, res, next) {
+  const cookieName = Settings.cookieName
+  const cookie = req.cookies && req.cookies[cookieName]
+  const signedCookie = req.signedCookies && req.signedCookies[cookieName]
+  let status
+  if (signedCookie) {
+    status = 'signed'
+  } else if (signedCookie === false) {
+    status = 'bad-signature'
+  } else if (cookie) {
+    status = 'unsigned'
+  } else {
+    status = 'none'
+  }
+  metrics.inc('session.cookie', 1, { status })
+  next()
+}
+
+module.exports = { middleware }

services/web/app/src/infrastructure/Server.js

@@ -16,6 +16,7 @@ const sessionsRedisClient = UserSessionsRedis.client()
 const SessionAutostartMiddleware = require('./SessionAutostartMiddleware')
 const AnalyticsManager = require('../Features/Analytics/AnalyticsManager')
 const session = require('express-session')
+const CookieMetrics = require('./CookieMetrics')
 const CustomSessionStore = require('./CustomSessionStore')
 const bodyParser = require('./BodyParserWrapper')
 const methodOverride = require('method-override')
@@ -165,6 +166,7 @@ const sessionSecrets = [
 ].filter(Boolean)
 
 webRouter.use(cookieParser(sessionSecrets))
+webRouter.use(CookieMetrics.middleware)
 SessionAutostartMiddleware.applyInitialMiddleware(webRouter)
 Modules.registerMiddleware(webRouter, 'sessionMiddleware', {
   store: sessionStore,

services/web/test/acceptance/src/CookieMetricsTests.js

@@ -0,0 +1,255 @@
+const Settings = require('@overleaf/settings')
+const { expect } = require('chai')
+const User = require('./helpers/User').promises
+const {
+  promises: { getMetric },
+  resetMetrics,
+} = require('./helpers/metrics')
+const cookieSignature = require('cookie-signature')
+
+async function getSessionCookieMetric(status) {
+  return getMetric(
+    line =>
+      line.includes('session_cookie') && line.includes(`status="${status}"`)
+  )
+}
+
+/*
+ * Modifies the session cookie by removing the existing signature and signing
+ * the cookie with a new secret.
+ */
+function modifyCookieSignature(originalCookie, newSecret) {
+  const [sessionKey] = originalCookie.value.slice(2).split('.')
+  return cookieSignature.sign(sessionKey, newSecret)
+}
+
+describe('Session cookie', function () {
+  before(async function () {
+    this.user = new User()
+  })
+
+  describe('with no session cookie', function () {
+    before(async function () {
+      resetMetrics()
+      const { response } = await this.user.doRequest('GET', '/login')
+      this.response = response
+    })
+
+    after(function () {
+      this.user.resetCookies()
+    })
+
+    it('should accept the request', function () {
+      expect(this.response.statusCode).to.equal(200)
+    })
+
+    it('should return a signed cookie', async function () {
+      const cookie = this.user.sessionCookie()
+      expect(cookie).to.exist
+      expect(cookie.key).to.equal(Settings.cookieName)
+      expect(cookie.value).to.match(/^s:/)
+    })
+
+    it('should sign the cookie with the current session secret', function () {
+      const cookie = this.user.sessionCookie()
+      const unsigned = cookieSignature.unsign(
+        cookie.value.slice(2), // strip the 's:' prefix
+        Settings.security.sessionSecret
+      )
+      expect(unsigned).not.to.be.false
+      expect(unsigned).to.match(/^[a-zA-Z0-9_-]+$/)
+    })
+
+    it('should record a "none" cookie metric', async function () {
+      const count = await getSessionCookieMetric('none')
+      expect(count).to.equal(1)
+    })
+  })
+
+  describe('with a signed session cookie', function () {
+    before(async function () {
+      // get the first cookie
+      await this.user.doRequest('GET', '/login')
+      this.firstCookie = this.user.sessionCookie()
+      // make a subsequent request
+      resetMetrics()
+      const { response } = await this.user.doRequest('GET', '/login')
+      this.response = response
+    })
+
+    after(function () {
+      this.user.resetCookies()
+    })
+
+    it('should accept the request', function () {
+      expect(this.response.statusCode).to.equal(200)
+    })
+
+    it('should return the same signed cookie', async function () {
+      const cookie = this.user.sessionCookie()
+      expect(cookie).to.exist
+      expect(cookie.key).to.equal(Settings.cookieName)
+      expect(cookie.value).to.equal(this.firstCookie.value)
+    })
+
+    it('should record a "signed" cookie metric', async function () {
+      const count = await getSessionCookieMetric('signed')
+      expect(count).to.equal(1)
+    })
+  })
+
+  describe('with a session cookie signed with the fallback session secret', function () {
+    before(async function () {
+      // get the first cookie
+      await this.user.doRequest('GET', '/login')
+      this.firstCookie = this.user.sessionCookie()
+      // sign the session key with the fallback secret
+      this.user.setSessionCookie(
+        's:' +
+          modifyCookieSignature(
+            this.firstCookie,
+            Settings.security.sessionSecretFallback
+          )
+      )
+      // make a subsequent request
+      resetMetrics()
+      const { response } = await this.user.doRequest('GET', '/login')
+      this.response = response
+    })
+
+    after(function () {
+      this.user.resetCookies()
+    })
+
+    it('should accept the request', async function () {
+      expect(this.response.statusCode).to.equal(200)
+    })
+
+    it('should return the cookie signed with the current secret', function () {
+      const cookie = this.user.sessionCookie()
+      expect(cookie).to.exist
+      expect(cookie.key).to.equal(Settings.cookieName)
+      expect(cookie.value).to.equal(this.firstCookie.value)
+    })
+
+    it('should record a "signed" cookie metric', async function () {
+      const count = await getSessionCookieMetric('signed')
+      expect(count).to.equal(1)
+    })
+  })
+
+  describe('with a session cookie signed with the upcoming session secret', function () {
+    before(async function () {
+      // get the first cookie
+      await this.user.doRequest('GET', '/login')
+      this.firstCookie = this.user.sessionCookie()
+      // sign the session key with the upcoming secret
+
+      this.user.setSessionCookie(
+        's:' +
+          modifyCookieSignature(
+            this.firstCookie,
+            Settings.security.sessionSecretUpcoming
+          )
+      )
+      // make a subsequent request
+      resetMetrics()
+      const { response } = await this.user.doRequest('GET', '/login')
+      this.response = response
+    })
+
+    after(function () {
+      this.user.resetCookies()
+    })
+
+    it('should accept the request', async function () {
+      expect(this.response.statusCode).to.equal(200)
+    })
+
+    it('should return the cookie signed with the current secret', function () {
+      const cookie = this.user.sessionCookie()
+      expect(cookie).to.exist
+      expect(cookie.key).to.equal(Settings.cookieName)
+      expect(cookie.value).to.equal(this.firstCookie.value)
+    })
+
+    it('should record a "signed" cookie metric', async function () {
+      const count = await getSessionCookieMetric('signed')
+      expect(count).to.equal(1)
+    })
+  })
+
+  describe('with a session cookie signed with an invalid secret', function () {
+    before(async function () {
+      // get the first cookie
+      await this.user.doRequest('GET', '/login')
+      this.firstCookie = this.user.sessionCookie()
+      // sign the session key with an invalid secret
+      this.user.setSessionCookie(
+        's:' + modifyCookieSignature(this.firstCookie, 'invalid-secret')
+      )
+      // make a subsequent request
+      resetMetrics()
+      const { response } = await this.user.doRequest('GET', '/login')
+      this.response = response
+    })
+
+    after(function () {
+      this.user.resetCookies()
+    })
+
+    it('should not reject the request', async function () {
+      expect(this.response.statusCode).to.equal(200)
+    })
+
+    it('should return a new cookie signed with the current secret', function () {
+      const cookie = this.user.sessionCookie()
+      expect(cookie).to.exist
+      expect(cookie.key).to.equal(Settings.cookieName)
+      const [sessionKey] = cookie.value.slice(2).split('.')
+      expect(sessionKey).not.to.equal(this.firstSessionKey)
+    })
+
+    it('should record a "bad-signature" cookie metric', async function () {
+      const count = await getSessionCookieMetric('bad-signature')
+      expect(count).to.equal(1)
+    })
+  })
+
+  describe('with an unsigned session cookie', function () {
+    before(async function () {
+      // get the first cookie
+      await this.user.doRequest('GET', '/login')
+      this.firstCookie = this.user.sessionCookie()
+      // use the session key without signing it
+      const [sessionKey] = this.firstCookie.value.slice(2).split('.')
+      this.firstSessionKey = sessionKey
+      this.user.setSessionCookie(sessionKey)
+      // make a subsequent request
+      resetMetrics()
+      const { response } = await this.user.doRequest('GET', '/login')
+      this.response = response
+    })
+
+    after(function () {
+      this.user.resetCookies()
+    })
+
+    it('should not reject the request', async function () {
+      expect(this.response.statusCode).to.equal(200)
+    })
+
+    it('should return a new cookie signed with the current secret', function () {
+      const cookie = this.user.sessionCookie()
+      expect(cookie).to.exist
+      expect(cookie.key).to.equal(Settings.cookieName)
+      const [sessionKey] = cookie.value.slice(2).split('.')
+      expect(sessionKey).not.to.equal(this.firstSessionKey)
+    })
+
+    it('should record an "unsigned" cookie metric', async function () {
+      const count = await getSessionCookieMetric('unsigned')
+      expect(count).to.equal(1)
+    })
+  })
+})

services/web/test/acceptance/src/helpers/metrics.js

@@ -1,5 +1,6 @@
 const { callbackify } = require('util')
 const request = require('./request')
+const metrics = require('@overleaf/metrics')
 
 async function getMetric(matcher) {
   const { body } = await request.promises.request('/metrics')
@@ -8,8 +9,16 @@ async function getMetric(matcher) {
   return parseInt(found.split(' ')[1], 0)
 }
 
+/* sets all metrics to zero
+   https://github.com/siimon/prom-client?tab=readme-ov-file#resetting-metrics
+*/
+function resetMetrics() {
+  metrics.register.resetMetrics()
+}
+
 module.exports = {
   getMetric: callbackify(getMetric),
+  resetMetrics,
   promises: {
     getMetric,
   },