feat(settings): expose the API rate limiter in the Security tab (#1338)
* feat(settings): expose the API rate limiter in the Security tab The general per-IP limiter had six settings in app_settings and a backend route to write them, and no screen. Installs ran on the code fallback — 300 requests per 15 minutes per IP — with no way to see it, which is how issue 1287 played out: a 546-photo gallery exhausted the budget for one viewer and the operator learned about the setting from a grep of the backend log. Security tab: a card with the six settings, the validation ranges the route enforces, a one-line explanation per field, and a note that the unit is the client IP — an office or household behind one NAT shares a budget, and behind a proxy TRUST_PROXY has to cover the proxy or every visitor shares its address. The tab's Save button saves the limiter through its own route. The limiter values are checked against the route's ranges before anything is written and the limiter is written first, so a rejected value cannot leave the password/session settings half-saved behind a failure toast. Backend, three things the screen needed: - The settings read fills the six keys with the code defaults when they have no row, so the form shows the budget in force rather than an empty field; the defaults live in one exported constant the limiter itself reads. - The write route upserts instead of updating: on a fresh install, which has no rows, the old UPDATE matched nothing and the route answered 200 while changing nothing. - The live limiter instances move into rateLimitService and the write route rebuilds them. express-rate-limit fixes windowMs when an instance is built — max and skip re-read the settings per request, the window does not — so a saved window used to apply only after a restart. The gates in server.js resolve the instance per request through the service's getters. A rebuild starts fresh counters, which on a settings change is acceptable. The limiters get explicit MemoryStores and a rebuild shuts the superseded ones down, because a store keeps a cleanup interval alive for as long as it exists and dropping the reference alone would leak one timer per save. Tests: the read surfaces defaults and honours the key filter; the write creates rows on a fresh database, the limiter sees the values immediately and hands the gates a fresh instance; existing rows are updated not duplicated; out-of-range values are rejected. The tab renders the values, edits through the hook state, carries the ranges, saves with the tab's button, and the pre-write validation accepts the bounds and rejects outside them and cleared fields. Relates to issue 1337 * docs(security): point the rate limiter doc at the Security tab and the upsert --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
co-authored by
Paul Nothaft
parent
834111fc66
commit
8017370271
@@ -10,7 +10,7 @@ const { formatBoolean } = require('../utils/dbCompat');
|
||||
const { adminAuth } = require('../middleware/auth');
|
||||
const { requirePermission, userHasAnyPermission } = require('../middleware/permissions');
|
||||
const { clearMaintenanceCache } = require('../middleware/maintenance');
|
||||
const { clearSettingsCache } = require('../services/rateLimitService');
|
||||
const { clearSettingsCache, initializeRateLimiters, RATE_LIMIT_DEFAULTS } = require('../services/rateLimitService');
|
||||
const {
|
||||
DEFAULT_PUBLIC_SITE_HTML,
|
||||
DEFAULT_PUBLIC_SITE_CSS,
|
||||
@@ -282,6 +282,14 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res)
|
||||
settingsObject.analytics_rybbit_api_key = '••••••••';
|
||||
}
|
||||
|
||||
// The general API rate limiter falls back to code defaults when a key has
|
||||
// no row, which is every fresh install. Surface those so the Security tab
|
||||
// shows the budget actually in force instead of an empty field (#1337).
|
||||
for (const [key, value] of Object.entries(RATE_LIMIT_DEFAULTS)) {
|
||||
if (settingsObject[key] === undefined && (!keysFilter || keysFilter.includes(key))) {
|
||||
settingsObject[key] = value;
|
||||
}
|
||||
}
|
||||
res.json(settingsObject);
|
||||
} catch (error) {
|
||||
errorResponse(res, error, 500, 'Failed to fetch settings');
|
||||
@@ -2067,10 +2075,19 @@ router.put('/security/rate-limit', adminAuth, requirePermission('settings.securi
|
||||
{ key: 'rate_limit_public_endpoints_only', value: rate_limit_public_endpoints_only }
|
||||
];
|
||||
|
||||
// Upsert, not update: a fresh install has no rate_limit_* rows, and a
|
||||
// plain update matched nothing there — the route answered 200 and
|
||||
// changed nothing (#1337).
|
||||
for (const { key, value } of settings) {
|
||||
await db('app_settings')
|
||||
.where('setting_key', key)
|
||||
.update({
|
||||
.insert({
|
||||
setting_key: key,
|
||||
setting_value: JSON.stringify(value),
|
||||
setting_type: 'security',
|
||||
updated_at: new Date()
|
||||
})
|
||||
.onConflict('setting_key')
|
||||
.merge({
|
||||
setting_value: JSON.stringify(value),
|
||||
updated_at: new Date()
|
||||
});
|
||||
@@ -2078,6 +2095,10 @@ router.put('/security/rate-limit', adminAuth, requirePermission('settings.securi
|
||||
|
||||
// Clear the rate limit settings cache to apply changes immediately
|
||||
clearSettingsCache();
|
||||
// max and skip re-read the settings per request, the window is fixed
|
||||
// per limiter instance: rebuild so a changed window applies now rather
|
||||
// than after a restart (#1337). Counters start fresh.
|
||||
await initializeRateLimiters();
|
||||
|
||||
// Log activity
|
||||
await logActivity('settings_updated',
|
||||
|
||||
@@ -1,9 +1,23 @@
|
||||
const rateLimit = require('express-rate-limit');
|
||||
const { MemoryStore } = require('express-rate-limit');
|
||||
const jwt = require('jsonwebtoken');
|
||||
const { db } = require('../database/db');
|
||||
const logger = require('../utils/logger');
|
||||
const { getAdminTokenFromRequest, getGalleryTokenFromRequest } = require('../utils/tokenUtils');
|
||||
|
||||
// What applies when app_settings has no row for a key — a fresh install has
|
||||
// none. Keyed by setting name so the admin settings read can surface the
|
||||
// same values (#1337): the Security tab must show the budget that is in
|
||||
// force, not an empty field that hides it.
|
||||
const RATE_LIMIT_DEFAULTS = Object.freeze({
|
||||
rate_limit_enabled: true,
|
||||
rate_limit_window_minutes: 15,
|
||||
rate_limit_max_requests: 300,
|
||||
rate_limit_auth_max_requests: 5,
|
||||
rate_limit_skip_authenticated: true,
|
||||
rate_limit_public_endpoints_only: false
|
||||
});
|
||||
|
||||
// Cache for rate limit settings
|
||||
let settingsCache = null;
|
||||
let cacheExpiry = 0;
|
||||
@@ -41,12 +55,12 @@ async function getRateLimitSettings() {
|
||||
// roughly twenty guests per window. An explicit app_settings value still
|
||||
// wins over this fallback.
|
||||
const config = {
|
||||
enabled: true,
|
||||
windowMinutes: 15,
|
||||
maxRequests: 300,
|
||||
authMaxRequests: 5,
|
||||
skipAuthenticated: true,
|
||||
publicEndpointsOnly: false
|
||||
enabled: RATE_LIMIT_DEFAULTS.rate_limit_enabled,
|
||||
windowMinutes: RATE_LIMIT_DEFAULTS.rate_limit_window_minutes,
|
||||
maxRequests: RATE_LIMIT_DEFAULTS.rate_limit_max_requests,
|
||||
authMaxRequests: RATE_LIMIT_DEFAULTS.rate_limit_auth_max_requests,
|
||||
skipAuthenticated: RATE_LIMIT_DEFAULTS.rate_limit_skip_authenticated,
|
||||
publicEndpointsOnly: RATE_LIMIT_DEFAULTS.rate_limit_public_endpoints_only
|
||||
};
|
||||
|
||||
settings.forEach(setting => {
|
||||
@@ -211,10 +225,11 @@ function shouldSkipRateLimit(req, config) {
|
||||
/**
|
||||
* Create dynamic rate limiter
|
||||
*/
|
||||
async function createRateLimiter() {
|
||||
async function createRateLimiter(store = new MemoryStore()) {
|
||||
const config = await getRateLimitSettings();
|
||||
|
||||
return rateLimit({
|
||||
store,
|
||||
windowMs: config.windowMinutes * 60 * 1000,
|
||||
max: async (req) => {
|
||||
// Refresh config for each request
|
||||
@@ -278,10 +293,11 @@ async function createRateLimiter() {
|
||||
* makes a 5-per-window per-IP budget safe behind NAT — a room full of guests on
|
||||
* one venue IP who all type the correct gallery password consume nothing.
|
||||
*/
|
||||
async function createAuthRateLimiter() {
|
||||
async function createAuthRateLimiter(store = new MemoryStore()) {
|
||||
const config = await getRateLimitSettings();
|
||||
|
||||
return rateLimit({
|
||||
store,
|
||||
windowMs: config.windowMinutes * 60 * 1000,
|
||||
// Read per request, like the general limiter, so a change to
|
||||
// rate_limit_auth_max_requests in admin Settings takes effect within the
|
||||
@@ -330,7 +346,40 @@ async function createAuthRateLimiter() {
|
||||
});
|
||||
}
|
||||
|
||||
// The live limiter instances. express-rate-limit fixes windowMs when an
|
||||
// instance is built — max and skip re-read the settings per request, the
|
||||
// window does not — so a saved window only takes effect on a rebuild. The
|
||||
// gates in server.js resolve the instance per request through the getters
|
||||
// below, and the settings route rebuilds after a save (#1337). Rebuilding
|
||||
// starts fresh counters; on a settings change that is acceptable.
|
||||
//
|
||||
// The stores are held explicitly because a MemoryStore runs a cleanup
|
||||
// interval for as long as it exists: dropping the limiter reference alone
|
||||
// would leave one more live timer and one more retained store per save.
|
||||
const current = { general: null, auth: null, stores: [] };
|
||||
|
||||
async function initializeRateLimiters() {
|
||||
const stores = [new MemoryStore(), new MemoryStore()];
|
||||
const general = await createRateLimiter(stores[0]);
|
||||
const auth = await createAuthRateLimiter(stores[1]);
|
||||
const superseded = current.stores;
|
||||
current.general = general;
|
||||
current.auth = auth;
|
||||
current.stores = stores;
|
||||
for (const store of superseded) {
|
||||
if (typeof store.shutdown === 'function') store.shutdown();
|
||||
}
|
||||
return current;
|
||||
}
|
||||
|
||||
const getGeneralLimiter = () => current.general;
|
||||
const getAuthLimiter = () => current.auth;
|
||||
|
||||
module.exports = {
|
||||
RATE_LIMIT_DEFAULTS,
|
||||
initializeRateLimiters,
|
||||
getGeneralLimiter,
|
||||
getAuthLimiter,
|
||||
getRateLimitSettings,
|
||||
clearSettingsCache,
|
||||
createRateLimiter,
|
||||
|
||||
Reference in New Issue
Block a user