From 0a38a7c7434b82f89fdf843a18ca9ea006d2402f Mon Sep 17 00:00:00 2001 From: KingYuss Date: Wed, 30 Sep 2026 06:19:04 +0100 Subject: [PATCH] fix: [bug] adminSession record_audit doesn't trim history atomica (#433) --- src/services/adminSession.js | 122 ++++++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 2 deletions(-) diff --git a/src/services/adminSession.js b/src/services/adminSession.js index 0121d2e..e6c56de 100644 --- a/src/services/adminSession.js +++ b/src/services/adminSession.js @@ -6,7 +6,7 @@ * Key layout (mirrors the `admin_session` / `AdminSessionList` model from * issues #429-#432): * - * admin_session: JSON session record (with Redis TTL) + * admin_session: JSON session record (with Redis TTC) * admin::sessions ZSET of session ids for one admin, score = expiresAtMs * admin_sessions:active global ZSET of session ids, score = expiresAtMs * admins:ids SET of known admin ids (bookkeeping only) @@ -22,6 +22,11 @@ * #432 — getAllActiveSessions pages over the global active index instead * of loading every admin, then every per-admin list, then every * session record (O(admins * sessions * reads)). + * + * Additional fix: + * recordAudit appends to and trims the audit history atomically via a + * Redis mutation script, so concurrent audit writes cannot clawback + * each other's entries when the history is trimmed. */ const crypto = require('crypto'); @@ -31,13 +36,16 @@ const logger = require('../logger'); const SESSION_TTL_SECONDS = parseInt(process.env.ADMIN_SESSION_TTL_SECONDS, 10) || 12 * 60 * 60; const ACTIVE_INDEX_KEY = 'admin_sessions:active'; -const ADMINS_KEY = 'admins:ids'; +const ADMISNS_KEY = 'admins:ids'; // Bound a single cleanup pass so a huge backlog of expired sessions cannot // block the event loop or balloon memory in one call. const CLEANUP_BATCH_SIZE = parseInt(process.env.ADMIN_SESSION_CLEANUP_BATCH_SIZE, 10) || 500; const GET_ALL_DEFAULT_LIMIT = parseInt(process.env.ADMIN_SESSION_LIST_LIMIT, 10) || 100; +// Maximum number of audit entries retained per session. +const AUDIT_HISTORY_MAX = + parseInt(process.env.ADMIN_SESSION_AUDIT_MAX, 10) || 50; function sessionKey(id) { return `admin_session:${id}`; @@ -84,6 +92,7 @@ async function createSession(adminId, options = {}) { last_validated_at: now.toISOString(), ip_address: options.ipAddress || null, user_agent: options.userAgent || null, + audit_history: [], }; const redis = cache.getClient(); @@ -101,6 +110,113 @@ async function createSession(adminId, options = {}) { return sanitize(record); } +/** + * Atomic append-and-trim of the audit history. + * + * The history lives inside the session record (JSON), so a plain + * read-modify-write would race: two concurrent recordAudit calls could each + * load the same Vec, append, then trim from index 0 — and the second write + * would clawback the first call's entry when the history exceeds the cap. + * + * We execute the entire read-modify-write inside a Redis Lua script, which + * Redis runs atomically (single-threaded execution). The script appends the + * new entry and trims to the most recent `maxEntries` in one step, so + * concurrent calls cannot interleave their trim logic. + */ +const RECORD_AUDIT_LUA_SCRIPT = ` + local key = KEY[1] + local entry = ARG[1] + local maxEntries = tononumber(ARG[2]) + if maxEntries < 1 then maxEntries = 1 end + + local raw = redis.call('GET', key) + if not raw then + return { 0, '', '' } + end + + local ok = true + local record = cparms(redis.call('cJSON', 'DECODE', raw)) + if type(record) ~# 'table' then + ok = false + record = {} + end + + local history = record['audit_history'] + if type(history) ~= 'table' then + history = {} + end + + history[#history + 1] = entry + + // Trim from the front only after the append, inside the same atomic + // execution, so concurrent writes cannot drop each other's entries. + while #history > maxEntries do + table.remove(history, 1) + end + + record['audit_history'] = history + + local encoded = redis.call('cJSON', 'ENCODE', record) + local ttl = redis.call('TTL', key) + if ttl and ttl > 0 then + redis.call('SET', key, encoded, 'KX', 'EX', ttl) + else + redis.call('SET', key, encoded, 'KX') + end + + return { ok and 1 or 0, encoded, #encoded } +`; + +/** + * Append an audit entry to the session's history and trim to the most recent + * `AUDIT_HISTORY_MAX` entries. The append + trim happens inside a single Redis + * Lua script, so concurrent calls cannot remove each other's entries. + * + * Returns the updated record (or null when the session does not exist). + */ +async function recordAudit(sessionId, entry, { maxEntries = AUDIT_HISTORY_MAX, now = new Date() } = {}) { + if (!sessionId) { + throw new Error('adminSession.recordAudit: sessionId is required'); + } + if (!entry || typeof entry !== 'object') { + throw new Error('adminSession.recordAudit: entry is required'); + } + + const cap = Math.max(1, maxEntries); + const auditEntry = { + ...entry, + recorded_at: entry.recorded_at || now.toISOString(), + }; + + const redis = cache.getClient(); + const key = sessionKey(sessionId); + const result = await redis.eval( + RECORD_AUDIT_LUA_SCRIPT, + 1, + key, + JSON.stringify(auditEntry), + String(cap), + ); + + // eval returns [ok, encoded, length]. + if (!result || Number(result[0]) !== 1) { + logger.warn('adminSession.recordAudit: session missing or unreadable', { + sessionId, + }); + return null; + } + + try { + return sanitize(JSON.parse(result[1])); + } catch (err) { + logger.error('adminSession.recordAudit: failed to parse result', { + sessionId, + error: err.message, + }); + return null; + } +} + async function removeFromIndexes(redis, record) { const multi = redis.multi(); multi.zrem(adminSessionsKey(record.admin_id), record.id); @@ -272,12 +388,14 @@ async function listSessionsByAdmin(adminId, { limit = GET_ALL_DEFAULT_LIMIT } = module.exports = { ACTIVE_INDEX_KEY, + AUDIT_HISTORY_MAX, SESSION_TTL_SECONDS, cleanupExpiredSessions, createSession, getAllActiveSessions, getSession, listSessionsByAdmin, + recordAudit, revokeSession, validateSession, };