From 1a6009c00cfb4bc720badda3fb60589d7db9bd61 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Thu, 23 Apr 2026 18:46:43 -0700 Subject: [PATCH] fix: anticsrf to store tokens in redis (#2834) * fix * fix: anticsrf to store tokens in redis --- .../src/clients/redis/redisSingleton.ts | 10 ++-- src/backend/src/middleware/anticsrf.js | 2 +- .../src/routers/auth/revoke-session.js | 2 +- src/backend/src/routers/down.js | 2 +- src/backend/src/routers/logout.js | 2 +- .../src/services/auth/AntiCSRFService.js | 60 +++++++++++-------- .../src/services/auth/AntiCSRFService.test.ts | 10 ++-- 7 files changed, 49 insertions(+), 39 deletions(-) diff --git a/src/backend/src/clients/redis/redisSingleton.ts b/src/backend/src/clients/redis/redisSingleton.ts index 683dd00b6..b59a11c56 100644 --- a/src/backend/src/clients/redis/redisSingleton.ts +++ b/src/backend/src/clients/redis/redisSingleton.ts @@ -4,7 +4,7 @@ import MockRedis from 'ioredis-mock'; const redisStartupRetryMaxDelayMs = 2000; const redisSlotsRefreshTimeoutMs = 5000; const redisConnectTimeoutMs = 10000; -const redisMaxRetriesPerRequest = 1; +const redisMaxRetriesPerRequest = 2; const redisBootRetryRegex = /Cluster(All)?FailedError|None of startup nodes is available/i; const formatRedisError = (error: unknown): string => { @@ -49,11 +49,11 @@ if ( process.env.REDIS_CONFIG ) { redisOpt = new Redis.Cluster(redisConfig, { dnsLookup: (address, callback) => callback(null, address), clusterRetryStrategy: (attempts) => Math.min(100 + (attempts * 100), redisStartupRetryMaxDelayMs), - retryDelayOnFailover: 500, - retryDelayOnClusterDown: 1000, - retryDelayOnTryAgain: 300, + retryDelayOnFailover: 50, + retryDelayOnClusterDown: 50, + retryDelayOnTryAgain: 50, slotsRefreshTimeout: redisSlotsRefreshTimeoutMs, - enableOfflineQueue: true, + enableOfflineQueue: false, redisOptions: { tls: {}, connectTimeout: redisConnectTimeoutMs, diff --git a/src/backend/src/middleware/anticsrf.js b/src/backend/src/middleware/anticsrf.js index d983e4a3f..aafd0eb4f 100644 --- a/src/backend/src/middleware/anticsrf.js +++ b/src/backend/src/middleware/anticsrf.js @@ -40,7 +40,7 @@ const anticsrf = options => async (req, res, next) => { err.write(res); return; } - const has = svc_antiCSRF.consume_token(req.user.uuid, req.body.anti_csrf); + const has = await svc_antiCSRF.consume_token(req.user.uuid, req.body.anti_csrf); if ( ! has ) { const err = APIError.create('anti-csrf-incorrect'); err.write(res); diff --git a/src/backend/src/routers/auth/revoke-session.js b/src/backend/src/routers/auth/revoke-session.js index d8822c9b2..f8c51fe38 100644 --- a/src/backend/src/routers/auth/revoke-session.js +++ b/src/backend/src/routers/auth/revoke-session.js @@ -37,7 +37,7 @@ module.exports = eggspress('/auth/revoke-session', { } const svc_antiCSRF = req.services.get('anti-csrf'); - if ( ! svc_antiCSRF.consume_token(actor.type.user.uuid, req.body.anti_csrf) ) { + if ( ! await svc_antiCSRF.consume_token(actor.type.user.uuid, req.body.anti_csrf) ) { return res.status(400).json({ message: 'incorrect anti-CSRF token' }); } diff --git a/src/backend/src/routers/down.js b/src/backend/src/routers/down.js index f26bc345d..b55c8c685 100644 --- a/src/backend/src/routers/down.js +++ b/src/backend/src/routers/down.js @@ -49,7 +49,7 @@ router.post('/down', express.json(), express.urlencoded({ extended: true }), con // check anti-csrf token const svc_antiCSRF = req.services.get('anti-csrf'); - if ( ! svc_antiCSRF.consume_token(req.user.uuid, req.body.anti_csrf) ) { + if ( ! await svc_antiCSRF.consume_token(req.user.uuid, req.body.anti_csrf) ) { return res.status(400).json({ message: 'incorrect anti-CSRF token' }); } diff --git a/src/backend/src/routers/logout.js b/src/backend/src/routers/logout.js index 84cbe275b..0b840131b 100644 --- a/src/backend/src/routers/logout.js +++ b/src/backend/src/routers/logout.js @@ -33,7 +33,7 @@ router.post('/logout', auth, express.json(), async (req, res, next) => { } // check anti-csrf token const svc_antiCSRF = req.services.get('anti-csrf'); - if ( ! svc_antiCSRF.consume_token(req.user.uuid, req.body.anti_csrf) ) { + if ( ! await svc_antiCSRF.consume_token(req.user.uuid, req.body.anti_csrf) ) { return res.status(400).json({ message: 'incorrect anti-CSRF token' }); } // delete cookie diff --git a/src/backend/src/services/auth/AntiCSRFService.js b/src/backend/src/services/auth/AntiCSRFService.js index e8b0fdbe7..5222aef54 100644 --- a/src/backend/src/services/auth/AntiCSRFService.js +++ b/src/backend/src/services/auth/AntiCSRFService.js @@ -20,21 +20,24 @@ const eggspress = require('../../api/eggspress'); const config = require('../../config'); const { subdomain } = require('../../helpers'); const BaseService = require('../BaseService'); -const { CircularQueue } = require('../../util/CircularQueue'); +const { redisClient } = require('../../clients/redis/redisSingleton'); + +const REDIS_KEY_PREFIX = 'anticsrf:'; +const MAX_TOKENS_PER_SESSION = 10; +const TOKEN_TTL_SECONDS = 60 * 60; +// Sub-millisecond tie-breaker so rapid-fire create_token calls get strictly +// increasing ZSET scores instead of falling back to lexical token ordering. +const SCORE_TIEBREAKER_MOD = 1000; /** * Class AntiCSRFService extends BaseService to manage and protect against Cross-Site Request Forgery (CSRF) attacks. -* It provides methods for generating, consuming, and verifying anti-CSRF tokens based on user sessions. +* Tokens are stored in Redis as a per-session sorted set (score = creation timestamp) +* so state is shared across backend instances. Only the most recent +* MAX_TOKENS_PER_SESSION tokens are retained; keys expire after TOKEN_TTL_SECONDS. */ class AntiCSRFService extends BaseService { - /** - * Initializes the AntiCSRFService instance and sets up the mapping - * between session IDs and their associated tokens. - * - * @returns {void} - */ _construct () { - this.map_session_to_tokens = {}; + this.score_tiebreaker_ = 0; } /** @@ -63,40 +66,47 @@ class AntiCSRFService extends BaseService { } // TODO: session uuid instead of user - const token = this.create_token(req.user.uuid); + const token = await this.create_token(req.user.uuid); res.send({ token }); })); } /** - * Creates a new anti-CSRF token for the specified session. - * If no token queue exists for the session, a new one is created. + * Creates a new anti-CSRF token for the specified session and stores it in Redis. + * Only the most recent MAX_TOKENS_PER_SESSION tokens are retained per session. * * @param {string} session - The session identifier - * @returns {string} The newly created token + * @returns {Promise} The newly created token */ - create_token (session) { - let tokens = this.map_session_to_tokens[session]; - if ( ! tokens ) { - tokens = new CircularQueue(10); - this.map_session_to_tokens[session] = tokens; - } + async create_token (session) { const token = this.generate_token_(); - tokens.push(token); + const key = this.redis_key_(session); + this.score_tiebreaker_ = (this.score_tiebreaker_ + 1) % SCORE_TIEBREAKER_MOD; + const score = Date.now() * SCORE_TIEBREAKER_MOD + this.score_tiebreaker_; + const pipeline = redisClient.pipeline(); + pipeline.zadd(key, score, token); + pipeline.zremrangebyrank(key, 0, -(MAX_TOKENS_PER_SESSION + 1)); + pipeline.expire(key, TOKEN_TTL_SECONDS); + await pipeline.exec(); return token; } /** * Attempts to consume (validate and remove) a token for the specified session. + * Uses an atomic ZREM so concurrent consumers can't double-spend a token. * * @param {string} session - The session identifier * @param {string} token - The token to consume - * @returns {boolean} True if the token was valid and consumed, false otherwise + * @returns {Promise} True if the token was valid and consumed, false otherwise */ - consume_token (session, token) { - const tokens = this.map_session_to_tokens[session]; - if ( ! tokens ) return false; - return tokens.maybe_consume(token); + async consume_token (session, token) { + if ( ! token ) return false; + const removed = await redisClient.zrem(this.redis_key_(session), token); + return removed > 0; + } + + redis_key_ (session) { + return `${REDIS_KEY_PREFIX}${session}`; } /** diff --git a/src/backend/src/services/auth/AntiCSRFService.test.ts b/src/backend/src/services/auth/AntiCSRFService.test.ts index 2c25a9592..346922cac 100644 --- a/src/backend/src/services/auth/AntiCSRFService.test.ts +++ b/src/backend/src/services/auth/AntiCSRFService.test.ts @@ -14,28 +14,28 @@ describe('AntiCSRFService', () => { // Do this several times, like a user would for ( let i = 0 ; i < 30 ; i++ ) { + const session = `session-${i}`; // Generate 30 tokens const tokens = []; for ( let j = 0 ; j < 30 ; j++ ) { - tokens.push(antiCSRFService.create_token('session')); + tokens.push(await antiCSRFService.create_token(session)); } // Only the last 10 should be valid const results_for_stale_tokens = []; for ( let j = 0 ; j < 20 ; j++ ) { - const result = antiCSRFService.consume_token('session', tokens[j]); + const result = await antiCSRFService.consume_token(session, tokens[j]); results_for_stale_tokens.push(result); } expect(results_for_stale_tokens.every(v => v === false)).toBe(true); // The last 10 should be valid const results_for_valid_tokens = []; for ( let j = 20 ; j < 30 ; j++ ) { - const result = antiCSRFService.consume_token('session', tokens[j]); + const result = await antiCSRFService.consume_token(session, tokens[j]); results_for_valid_tokens.push(result); } expect(results_for_valid_tokens.every(v => v === true)).toBe(true); // A completely arbitrary token should not be valid - expect(antiCSRFService.consume_token('session', 'arbitrary')).toBe(false); + expect(await antiCSRFService.consume_token(session, 'arbitrary')).toBe(false); } }); }); -