diff --git a/services/web/app/src/infrastructure/CookieMetrics.js b/services/web/app/src/infrastructure/CookieMetrics.js new file mode 100644 index 0000000000..9e35b51b6f --- /dev/null +++ b/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 } diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 02a403f843..0830a6f89d 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/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, diff --git a/services/web/test/acceptance/src/CookieMetricsTests.js b/services/web/test/acceptance/src/CookieMetricsTests.js new file mode 100644 index 0000000000..40e0a1a513 --- /dev/null +++ b/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) + }) + }) +}) diff --git a/services/web/test/acceptance/src/helpers/metrics.js b/services/web/test/acceptance/src/helpers/metrics.js index b13b1afd0b..6cc4dc2d55 100644 --- a/services/web/test/acceptance/src/helpers/metrics.js +++ b/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, },