From 662516a5ad2a0aadd87dfff3fba4f2456e88a69d Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:53:53 +0200 Subject: [PATCH] fix: retain revocations for tokens without expiry --- .../integration/tokenRevocationExpiry.test.js | 96 +++++++++++++++++++ .../__tests__/routes/logoutRevocation.test.js | 75 +++++++++++++++ .../utils/tokenRevocation.forgery.test.js | 2 +- .../core/211_revocations_without_expiry.js | 26 +++++ backend/src/database/db.js | 2 +- backend/src/routes/adminAuth.js | 7 +- backend/src/routes/auth.js | 11 ++- backend/src/routes/customerAuth.js | 4 +- backend/src/utils/tokenRevocation.js | 28 ++++-- 9 files changed, 234 insertions(+), 17 deletions(-) create mode 100644 backend/__tests__/integration/tokenRevocationExpiry.test.js create mode 100644 backend/__tests__/routes/logoutRevocation.test.js create mode 100644 backend/migrations/core/211_revocations_without_expiry.js diff --git a/backend/__tests__/integration/tokenRevocationExpiry.test.js b/backend/__tests__/integration/tokenRevocationExpiry.test.js new file mode 100644 index 00000000..b2699fda --- /dev/null +++ b/backend/__tests__/integration/tokenRevocationExpiry.test.js @@ -0,0 +1,96 @@ +const knex = require('knex'); +const jwt = require('jsonwebtoken'); +const { randomUUID } = require('crypto'); +const migration = require('../../migrations/core/211_revocations_without_expiry'); + +for (const client of ['sqlite3', 'pg']) { + const enabled = client !== 'pg' || process.env.PICPEAK_PG_TEST_URL; + (enabled ? describe : describe.skip)(`token revocation expiry (${client})`, () => { + let db, owner, schema, revocation; + const sign = claims => jwt.sign({ id: 1, type: 'admin', jti: randomUUID(), ...claims }, process.env.JWT_SECRET); + + beforeAll(async () => { + if (client === 'pg') { + schema = `revocation_${randomUUID().replace(/-/g, '')}`; + owner = knex({ client, connection: process.env.PICPEAK_PG_TEST_URL }); + await owner.schema.createSchema(schema); + db = knex({ client, connection: process.env.PICPEAK_PG_TEST_URL, searchPath: [schema] }); + } else { + db = knex({ client, connection: { filename: ':memory:' }, useNullAsDefault: true }); + } + // Exercise the upgrade from the real legacy NOT NULL schema as well as + // repeated migration runs, without sharing another test's database. + await require('../../migrations/legacy/017_add_token_revocation_tables').up(db); + await db('revoked_tokens').insert({ token_id: 'existing', expires_at: '2099-01-01T00:00:00.000Z' }); + await migration.up(db); + await migration.up(db); + jest.resetModules(); + jest.doMock('../../src/database/db', () => ({ db })); + revocation = require('../../src/utils/tokenRevocation'); + }); + + afterAll(async () => { + await db?.destroy(); + if (owner) { await owner.schema.dropSchema(schema, true); await owner.destroy(); } + jest.dontMock('../../src/database/db'); + }); + + it('preserves existing revocations and their unique key during upgrade', async () => { + expect(await db('revoked_tokens').where({ token_id: 'existing' }).first()).toBeTruthy(); + await expect(db('revoked_tokens').insert({ token_id: 'existing', expires_at: null })).rejects.toThrow(); + }); + + it.each([true, false])('permanently revokes a token without exp (jti: %s)', async withJti => { + const token = sign(withJti ? {} : { jti: undefined }); + const payload = jwt.verify(token, process.env.JWT_SECRET); + expect(await revocation.isTokenRevoked(payload)).toBe(false); + expect(await revocation.revokeToken(token, 'logout')).toBe(true); + expect(await revocation.revokeToken(token, 'logout')).toBe(true); + await revocation.cleanupExpiredRevocations(); + expect(await revocation.isTokenRevoked(payload)).toBe(true); + const rows = await db('revoked_tokens').where({ token_id: revocation.buildTokenId(payload) }); + expect(rows).toHaveLength(1); + expect(rows[0].expires_at).toBeNull(); + }); + + it('cleans up expired revocations and retains future ones', async () => { + const expired = sign({ exp: Math.floor(Date.now() / 1000) - 60 }); + const future = sign({ exp: Math.floor(Date.now() / 1000) + 3600 }); + expect(await revocation.revokeToken(expired, 'logout')).toBe(true); + expect(await revocation.revokeToken(future, 'logout')).toBe(true); + await revocation.cleanupExpiredRevocations(); + expect(await revocation.isTokenRevoked(jwt.decode(expired))).toBe(false); + expect(await revocation.isTokenRevoked(jwt.decode(future))).toBe(true); + }); + + it.each([true, false])('upgrades an expiring entry with the same key permanently (jti: %s)', async withJti => { + const claims = { id: 99, iat: Math.floor(Date.now() / 1000), jti: withJti ? randomUUID() : undefined }; + const expiring = sign({ ...claims, exp: claims.iat - 60 }); + const permanent = sign(claims); + expect(await revocation.revokeToken(expiring, 'logout')).toBe(true); + expect(await revocation.revokeToken(permanent, 'logout')).toBe(true); + expect(await revocation.revokeToken(expiring, 'logout')).toBe(true); + await revocation.cleanupExpiredRevocations(); + expect(await revocation.isTokenRevoked(jwt.decode(permanent))).toBe(true); + }); + + it('retains a signed token whose numeric expiry cannot fit a database timestamp', async () => { + const token = sign({ exp: 1e100 }); + expect(await revocation.revokeToken(token, 'logout')).toBe(true); + await revocation.cleanupExpiredRevocations(); + expect(await revocation.isTokenRevoked(jwt.decode(token))).toBe(true); + }); + + it('refuses a rollback that would remove permanent revocations', async () => { + await expect(migration.down(db)).rejects.toThrow('permanent token revocations'); + expect((await db('revoked_tokens').columnInfo('expires_at')).nullable).toBe(true); + // A rollback with only expiring records remains supported and reversible. + await db('revoked_tokens').whereNull('expires_at').delete(); + await migration.down(db); + await migration.down(db); + expect((await db('revoked_tokens').columnInfo('expires_at')).nullable).toBe(false); + await migration.up(db); + expect(await db('revoked_tokens').where({ token_id: 'existing' }).first()).toBeTruthy(); + }); + }); +} diff --git a/backend/__tests__/routes/logoutRevocation.test.js b/backend/__tests__/routes/logoutRevocation.test.js new file mode 100644 index 00000000..df58e4bc --- /dev/null +++ b/backend/__tests__/routes/logoutRevocation.test.js @@ -0,0 +1,75 @@ +const request = require('supertest'); +const jwt = require('jsonwebtoken'); +const { randomUUID } = require('crypto'); +const { bootCrmDb, seedMinimal, assignAdminRole, buildRouteApp } = require('../integration/helpers/crmDb'); + +let db, cleanup, adminId, customerId, eventId, apps, revocation; +const slug = 'logout-revocation'; +const cases = [ + ['auth', '/logout', 'admin', 'admin_token'], + ['auth', '/gallery/logout', 'gallery', `gallery_token_${slug}`], + ['customerAuth', '/logout', 'customer', 'customer_token'], + ['adminAuth', '/logout', 'admin', 'admin_token'], +]; +const sign = type => jwt.sign({ + type, ...(type === 'admin' ? { id: adminId } : type === 'customer' ? { customerId } : { eventId, eventSlug: slug }), + jti: randomUUID(), +}, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ adminId, customerId } = await seedMinimal(db)); + await assignAdminRole(db, adminId); + const event = await require('../../src/services/eventCreationService').createEvent({ + event_type: 'wedding', event_name: 'Logout revocation', event_date: '2026-10-01', + slug, password: 'Logout-Strong-Password-924!', expiration_days: 30, + customer_email: 'customer@example.test', admin_email: 'admin@example.test', + }, { actor: { id: adminId }, source: 'v1' }); + eventId = event.id; + await db('events').where({ id: eventId }).update({ slug }); + revocation = require('../../src/utils/tokenRevocation'); + apps = Object.fromEntries(['auth', 'adminAuth', 'customerAuth'].map(name => [ + name, buildRouteApp('/', require(`../../src/routes/${name}`)), + ])); +}); +afterEach(() => jest.restoreAllMocks()); +afterAll(async () => { + await require('../../src/services/serviceShutdown').stopServices(); + if (cleanup) await cleanup(); +}); + +it.each(cases)('%s%s revokes a no-expiry %s cookie session', async (route, path, type, cookie) => { + const token = sign(type); + const sessionApp = type === 'customer' ? apps.customerAuth : apps.auth; + const sessionPath = type === 'gallery' ? `/session?slug=${slug}` : '/session'; + const session = () => request(sessionApp).get(sessionPath).set('Cookie', `${cookie}=${token}`); + const before = await session(); + expect(before.status).toBe(200); + if (type !== 'customer') expect(before.body.valid).toBe(true); + + const res = await request(apps[route]).post(path).set('Cookie', `${cookie}=${token}`).send({ slug }); + expect(res.status).toBe(200); + expect(res.headers['set-cookie'].some(value => value.startsWith(`${cookie}=;`))).toBe(true); + await revocation.cleanupExpiredRevocations(); + expect(await revocation.isTokenRevoked(jwt.decode(token))).toBe(true); + const after = await session(); + if (type === 'customer') expect(after.status).toBe(401); + else expect(after.body.valid).toBe(false); +}); + +it.each(cases)('%s%s reports failed persistence for %s logout and clears its cookie', async (route, path, type, cookie) => { + const token = sign(type); + const realQuery = db.client.query; + jest.spyOn(db.client, 'query').mockImplementation(function (connection, query) { + if (/^insert into [`"]revoked_tokens[`"]/.test(query.sql)) { + return Promise.reject(new Error('simulated revocation write failure')); + } + return realQuery.call(this, connection, query); + }); + const res = await request(apps[route]).post(path).set('Cookie', `${cookie}=${token}`).send({ slug }); + expect(res.status).toBe(500); + expect(res.body.error).toBeTruthy(); + expect(res.body.message).not.toBe('Logged out successfully'); + expect(res.headers['set-cookie'].some(value => value.startsWith(`${cookie}=;`))).toBe(true); + expect(await revocation.isTokenRevoked(jwt.decode(token))).toBe(false); +}); diff --git a/backend/__tests__/utils/tokenRevocation.forgery.test.js b/backend/__tests__/utils/tokenRevocation.forgery.test.js index a5b54584..8c3f76ff 100644 --- a/backend/__tests__/utils/tokenRevocation.forgery.test.js +++ b/backend/__tests__/utils/tokenRevocation.forgery.test.js @@ -20,7 +20,7 @@ jest.mock('../../src/database/db', () => { const dbFn = () => ({ insert(row) { inserted.push(row); - return { onConflict: () => ({ ignore: async () => undefined }) }; + return { onConflict: () => ({ ignore: async () => undefined, merge: async () => undefined }) }; }, }); return { db: dbFn }; diff --git a/backend/migrations/core/211_revocations_without_expiry.js b/backend/migrations/core/211_revocations_without_expiry.js new file mode 100644 index 00000000..714a7864 --- /dev/null +++ b/backend/migrations/core/211_revocations_without_expiry.js @@ -0,0 +1,26 @@ +/** Non-expiring JWTs need revocation records that cleanup never removes. */ +exports.up = async function (knex) { + if (!await knex.schema.hasTable('revoked_tokens')) return; + if (!await knex.schema.hasColumn('revoked_tokens', 'expires_at')) return; + const column = await knex('revoked_tokens').columnInfo('expires_at'); + if (!column.nullable) { + await knex.schema.alterTable('revoked_tokens', table => { + table.timestamp('expires_at').nullable().alter(); + }); + } +}; + +exports.down = async function (knex) { + if (!await knex.schema.hasTable('revoked_tokens')) return; + if (!await knex.schema.hasColumn('revoked_tokens', 'expires_at')) return; + // Refuse to discard permanent revocations or silently give them a TTL. + if (await knex('revoked_tokens').whereNull('expires_at').first()) { + throw new Error('Cannot roll back while permanent token revocations exist'); + } + const column = await knex('revoked_tokens').columnInfo('expires_at'); + if (column.nullable) { + await knex.schema.alterTable('revoked_tokens', table => { + table.timestamp('expires_at').notNullable().alter(); + }); + } +}; diff --git a/backend/src/database/db.js b/backend/src/database/db.js index 11d0257c..59c6d8fb 100644 --- a/backend/src/database/db.js +++ b/backend/src/database/db.js @@ -421,7 +421,7 @@ async function initializeDatabase() { table.integer('user_id').nullable(); // User who owned the token table.string('token_type', 20); // admin, gallery, etc. table.timestamp('revoked_at').defaultTo(db.fn.now()); - table.timestamp('expires_at').notNullable(); // When token would have expired + table.timestamp('expires_at').nullable(); // NULL retains tokens without a known expiry table.string('reason', 100); // password_change, logout, compromised, etc. table.text('metadata'); // Additional JSON data diff --git a/backend/src/routes/adminAuth.js b/backend/src/routes/adminAuth.js index fef31a1c..b2abc2c5 100644 --- a/backend/src/routes/adminAuth.js +++ b/backend/src/routes/adminAuth.js @@ -182,16 +182,17 @@ router.post('/logout', adminAuth, handleAsync(async (req, res) => { // old header-only read skipped revocation entirely for cookie-based logout, // leaving the JWT valid until expiry while reporting a successful logout. const token = req.token; + clearAdminAuthCookie(res); if (token) { // End the in-memory session AND revoke the JWT (GHSA-cjqh) — the token // is otherwise valid until expiry, so photoAuth/adminAuth would keep // honouring it after logout. isTokenRevoked() checks this store. endSession(token); const { revokeToken } = require('../utils/tokenRevocation'); - await revokeToken(token, 'logout'); + if (!await revokeToken(token, 'logout')) { + throw new Error('Token revocation failed'); + } } - // Clear the auth cookie so the browser stops sending the (now revoked) JWT. - clearAdminAuthCookie(res); // Log activity await logActivity('admin_logout', diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index bc58cd1c..6e5c4e17 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -356,7 +356,9 @@ router.post('/logout', async (req, res) => { if (token) { // Revoke the token so it can't be reused, then end the session - await revokeToken(token, 'user_logout'); + if (!await revokeToken(token, 'user_logout')) { + throw new Error('Token revocation failed'); + } endSession(token); try { @@ -400,6 +402,8 @@ router.post('/logout', async (req, res) => { res.json({ message: 'Logged out successfully', ...(ssoLogoutUrl ? { ssoLogoutUrl } : {}) }); } catch (error) { + clearAdminAuthCookie(res); + clearGalleryAuthCookies(res); errorResponse(res, error, 500, 'Logout failed'); } }); @@ -714,11 +718,14 @@ router.post('/gallery/logout', async (req, res) => { const { slug } = req.body || {}; const token = getGalleryTokenFromRequest(req, slug); if (token) { - await revokeToken(token, 'gallery_logout'); + if (!await revokeToken(token, 'gallery_logout')) { + throw new Error('Token revocation failed'); + } } clearGalleryAuthCookies(res, slug); res.json({ message: 'Logged out successfully' }); } catch (error) { + clearGalleryAuthCookies(res, req.body?.slug); errorResponse(res, error, 500, 'Logout failed'); } }); diff --git a/backend/src/routes/customerAuth.js b/backend/src/routes/customerAuth.js index 05a589a7..9f7134d1 100644 --- a/backend/src/routes/customerAuth.js +++ b/backend/src/routes/customerAuth.js @@ -181,7 +181,9 @@ router.post('/logout', async (req, res) => { try { const token = getCustomerTokenFromRequest(req); if (token) { - await revokeToken(token, 'user_logout'); + if (!await revokeToken(token, 'user_logout')) { + throw new Error('Token revocation failed'); + } } clearCustomerAuthCookie(res); res.json({ message: 'Logged out successfully' }); diff --git a/backend/src/utils/tokenRevocation.js b/backend/src/utils/tokenRevocation.js index 64b2a89e..06b9c4ee 100644 --- a/backend/src/utils/tokenRevocation.js +++ b/backend/src/utils/tokenRevocation.js @@ -6,6 +6,7 @@ const jwt = require('jsonwebtoken'); const { db } = require('../database/db'); const logger = require('./logger'); +const MAX_SQL_EXPIRY_SECONDS = Date.parse('9999-12-31T23:59:59Z') / 1000; /** * Add a token to the revocation list @@ -52,20 +53,28 @@ async function revokeToken(token, reason, metadata = {}) { // undefined/string slip through and cause an INSERT type error. const userIdNumeric = Number.isInteger(payload.id) ? payload.id : null; - // onConflict.ignore: revoking an already-revoked token is a no-op, - // not an error. Hits the unique (token_id) index when the same JWT - // is logged out twice (e.g. duplicate /logout from two tabs, or a - // session-expiry path that races with an explicit logout). The - // previous insert was authoritative; nothing to do. - await db('revoked_tokens').insert({ + // JWT permits a missing exp. Keep that revocation permanently: an + // arbitrary fallback TTL would make the token usable again after cleanup. + // Retain unrepresentable expiries too, using the common SQL/ISO date range. + const expiresAt = Number.isFinite(payload.exp) + && payload.exp >= 0 && payload.exp <= MAX_SQL_EXPIRY_SECONDS + ? new Date(Math.ceil(payload.exp * 1000)).toISOString() + : null; + + // Duplicate logouts are idempotent. A permanent revocation must also + // upgrade an existing expiring entry with the same legacy key or jti; + // logging out an expiring token must never shorten that retention again. + const insert = db('revoked_tokens').insert({ token_id: buildTokenId(payload), user_id: userIdNumeric, token_type: payload.type, revoked_at: new Date().toISOString(), - expires_at: new Date(payload.exp * 1000).toISOString(), + expires_at: expiresAt, reason, metadata: JSON.stringify(metadata) - }).onConflict('token_id').ignore(); + }).onConflict('token_id'); + if (expiresAt === null) await insert.merge({ expires_at: null }); + else await insert.ignore(); logger.info('Token revoked', { userId: payload.id ?? payload.customerId ?? null, @@ -131,6 +140,7 @@ async function revokeAllUserTokens(userId, reason) { async function cleanupExpiredRevocations() { try { const deleted = await db('revoked_tokens') + .whereNotNull('expires_at') .where('expires_at', '<', new Date().toISOString()) .delete(); @@ -156,4 +166,4 @@ module.exports = { buildTokenId, revokeAllUserTokens, cleanupExpiredRevocations, initializeRevocationCleanup -}; \ No newline at end of file +};