Fix for rate limiter only trusts X-Forwarded-For when the socket is

loopback, and the admin nginx sets that header to its own $remote_addr
This commit is contained in:
Jamie Curnow committed 2026-11-09 22:00:26 +10:00
1 parent 36b3434b9c
commit 0cde59a8ad
3 files changed
+74 -12

No files matched your search

+40 -11
View File
@@ -4,24 +4,57 @@ import { isCI } from "../config.js";
* In-memory sliding window rate limiter for authentication endpoints
*/
const ipRequests = new Map();
// One store per limiter, so separate endpoints never share buckets
const stores = new Set();
// Clean up old entries every 2 minutes
const cleanupTimer = setInterval(() => {
const now = Date.now();
for (const [ip, record] of ipRequests.entries()) {
if (now - record.startTime > record.windowMs * 2) {
ipRequests.delete(ip);
for (const store of stores) {
for (const [key, record] of store.entries()) {
if (now - record.startTime > record.windowMs * 2) {
store.delete(key);
}
}
}
}, 120000);
cleanupTimer.unref();
/**
* The client IP as seen by the backend. X-Forwarded-For is only honoured when the socket is
* loopback, which is the admin nginx in this container; it sets the header to its own
* $remote_addr after realip processing, so the first entry is the resolved client.
*
* @param {Object} req
* @returns {String}
*/
export const getClientIp = (req) => {
const socketAddr = req.socket?.remoteAddress || "unknown";
const isTrustedLocalProxy = socketAddr === "127.0.0.1" || socketAddr === "::1" || socketAddr === "::ffff:127.0.0.1";
const forwardedFor = isTrustedLocalProxy ? req.headers["x-forwarded-for"]?.split(",")[0].trim() : null;
return forwardedFor || socketAddr;
};
/**
* @param {Object} [options]
* @param {Number} [options.windowMs]
* @param {Number} [options.max]
* @param {String} [options.message]
* @param {Boolean} [options.skipSuccessfulRequests]
* @param {Function} [options.keyGenerator] (req, clientIp) => String. Defaults to the client IP.
* Including the target account in the key stops every
* user being locked out when many clients share an IP
* (e.g. the admin UI behind another reverse proxy).
*/
export default function createRateLimiter(options = {}) {
const windowMs = options.windowMs || 60 * 1000; // 1 minute default
const max = options.max || 10; // 10 requests default
const message = options.message || "Too many attempts from this IP, please try again later.";
const skipSuccessfulRequests = options.skipSuccessfulRequests !== false;
const keyGenerator = options.keyGenerator || ((_req, clientIp) => clientIp);
const store = new Map();
stores.add(store);
return (req, res, next) => {
// Bypass rate limiting in CI, test environments, or when explicitly disabled
@@ -34,14 +67,10 @@ export default function createRateLimiter(options = {}) {
return next();
}
const socketAddr = req.socket?.remoteAddress || "unknown";
const isTrustedLocalProxy =
socketAddr === "127.0.0.1" || socketAddr === "::1" || socketAddr === "::ffff:127.0.0.1";
const forwardedFor = isTrustedLocalProxy ? req.headers["x-forwarded-for"]?.split(",")[0].trim() : null;
const clientIp = forwardedFor || socketAddr;
const key = keyGenerator(req, getClientIp(req));
const now = Date.now();
let record = ipRequests.get(clientIp);
let record = store.get(key);
if (!record || now - record.startTime > windowMs) {
record = {
@@ -49,7 +78,7 @@ export default function createRateLimiter(options = {}) {
startTime: now,
windowMs,
};
ipRequests.set(clientIp, record);
store.set(key, record);
if (skipSuccessfulRequests) {
const currentRecord = record;
+30 -1
View File
@@ -6,11 +6,40 @@ import apiValidator from "../lib/validator/api.js";
import { debug, express as logger } from "../logger.js";
import { getValidationSchema } from "../schema/index.js";
// Keyed on IP + target account rather than IP alone: when the admin UI sits behind another
// proxy every client can resolve to the same IP, and an IP-only bucket would let anyone lock
// out every user with a handful of bad logins. Brute force against one account is still
// capped per IP.
const tokenRateLimiter = createRateLimiter({
windowMs: 60 * 1000,
max: 10,
message: "Too many login attempts. Please try again in 1 minute.",
skipSuccessfulRequests: true,
keyGenerator: (req, clientIp) => {
const identity = typeof req.body?.identity === "string" ? req.body.identity.trim().toLowerCase() : "";
return `${clientIp}|${identity}`;
},
});
// Keyed on the user the challenge belongs to, not the challenge token itself, so fetching
// fresh challenge tokens doesn't reset the code-guessing budget. The payload is read without
// verifying the signature; verify2FA does that. A forged token only affects buckets for the
// sender's own IP.
const challengeUserId = (token) => {
try {
const payload = JSON.parse(Buffer.from(String(token).split(".")[1], "base64url").toString());
return String(payload?.attrs?.id ?? "");
} catch {
return "";
}
};
const twoFactorRateLimiter = createRateLimiter({
windowMs: 60 * 1000,
max: 10,
message: "Too many login attempts. Please try again in 1 minute.",
skipSuccessfulRequests: true,
keyGenerator: (req, clientIp) => `${clientIp}|${challengeUserId(req.body?.challenge_token)}`,
});
const router = express.Router({
@@ -72,7 +101,7 @@ router
*
* Verify 2FA code and get full token
*/
.post(tokenRateLimiter, async (req, res, next) => {
.post(twoFactorRateLimiter, async (req, res, next) => {
try {
const { challenge_token, code } = await apiValidator(getValidationSchema("/tokens/2fa", "post"), req.body);
const result = await internalToken.verify2FA(challenge_token, code);
+4
View File
@@ -77,6 +77,10 @@ http {
# Real IP Determination
# Loopback: lets an NPM proxy host forwarding to 127.0.0.1 (e.g. the admin UI on :81)
# pass the real client IP through instead of every request appearing as 127.0.0.1
set_real_ip_from 127.0.0.1;
set_real_ip_from ::1;
# Local subnets:
set_real_ip_from 10.0.0.0/8;
set_real_ip_from 172.16.0.0/12; # Includes Docker subnet