fix(security): apply per-IP rate limiting to credential endpoints
The five authRateLimiter registrations were inert for the same reason the
general one was -- registered below the error handler. Auth endpoints have
never had an IP limit; the 5-attempt behaviour QA observed is the per-account
lockout in authSecurity.js, which is a different mechanism and is untouched.
They could not simply be activated: app.use('/api/auth', ...) is a prefix, so
a 5-per-window budget would have covered GET /api/auth/session and
POST /api/auth/password-strength, which the frontend calls far more than five
times per window. That locks users out.
The real surface was enumerated by loading the routers and walking
router.stack rather than grepping, which showed two of the five registrations
pointed at routes that do not exist: adminAuth.js has no /login (admin login
is POST /api/auth/admin/login) and there is no /api/gallery/:slug/verify
(gallery verify is POST /api/auth/gallery/verify).
Now limited, on exact method+path: admin login, admin MFA verify, gallery
password verify, share-login, client PIN, setup verify-token, setup admin,
customer login, customer password-reset. Deliberately unlimited: session
checks, password-strength, logouts, authenticated change-password, the SSO
round-trip (a 429 on the callback breaks login from shared corporate IPs),
and one-time invite/accept-invite links.
Two choices carry the design. skipSuccessfulRequests means only failed
attempts spend budget, which is what makes 5-per-IP survivable behind NAT --
ten guests on one venue wifi all typing the correct gallery password consume
nothing -- and means a legitimate admin cannot be locked out by their own
success. And the limiter keeps its own rateLimit() instance, hence its own
store and its own per-IP bucket, with the general gate's auth exemption left
in place: sharing a counter is exactly the lockout described above.
Patterns are case-insensitive because Express's case-sensitive routing is off
by default, so POST /api/auth/admin/LOGIN reaches the login handler and a
case-sensitive pattern would have been a free bypass.
max is now read per request, so the Settings UI's rate_limit_auth_max_requests
applies without a restart, matching the general limiter.
Tests prove both directions: each credential endpoint 429s on attempt 6 with
the response shape the four login pages already branch on, each benign
endpoint still returns 200 after 40 calls, the two buckets are independent in
both directions, and 30 consecutive successful logins consume no budget.
Refs testplan REPORT.md, rate-limiter gap.
This commit is contained in:
+19
-17
@@ -90,6 +90,7 @@ const { sessionTimeoutMiddleware } = require('./src/middleware/sessionTimeout');
|
||||
const { errorHandler, notFoundHandler } = require('./src/middleware/errorHandler');
|
||||
const { createRateLimiter, createAuthRateLimiter } = require('./src/services/rateLimitService');
|
||||
const { createApiRateLimitGate } = require('./src/middleware/apiRateLimitGate');
|
||||
const { createAuthRateLimitGate } = require('./src/middleware/authRateLimitGate');
|
||||
const { getPublicSitePayload } = require('./src/services/publicSiteService');
|
||||
const cookieParser = require('cookie-parser');
|
||||
const {
|
||||
@@ -497,24 +498,19 @@ async function initializeRateLimiters() {
|
||||
generalRateLimiter = await createRateLimiter();
|
||||
authRateLimiter = await createAuthRateLimiter();
|
||||
|
||||
// The general limiter is NOT registered here — an app.use() at this point
|
||||
// runs after the routers, the /api 404 handler and the error handler are
|
||||
// already on the stack, so it can never see a request. It is reached through
|
||||
// apiRateLimitGate below instead, which is registered ahead of the routers
|
||||
// and reads this variable per request.
|
||||
// Neither limiter is registered here — an app.use() at this point runs after
|
||||
// the routers, the /api 404 handler and the error handler are already on the
|
||||
// stack, so it can never see a request. Both are reached through their gates
|
||||
// below, which are registered ahead of the routers and read these variables
|
||||
// per request.
|
||||
//
|
||||
// These authRateLimiter registrations have the same problem and are equally
|
||||
// inert. They are left as-is deliberately: `/api/auth` is a prefix, and the
|
||||
// limiter's 5-per-window budget applies to every route under it — including
|
||||
// GET /api/auth/session and POST /api/auth/password-strength, which the
|
||||
// frontend calls far more often than five times per window. Activating them
|
||||
// as written would lock legitimate users out, so wiring per-IP auth limiting
|
||||
// up properly is a separate change.
|
||||
app.use('/api/auth', authRateLimiter);
|
||||
app.use('/api/gallery/:slug/verify', authRateLimiter);
|
||||
app.use('/api/admin/auth/login', authRateLimiter);
|
||||
app.use('/api/setup/admin', authRateLimiter);
|
||||
app.use('/api/setup/verify-token', authRateLimiter);
|
||||
// The five prefix registrations of authRateLimiter that used to live here
|
||||
// were inert for that reason, and could not simply be moved up either: the
|
||||
// widest of them mounted on the whole /api/auth router, so the 5-per-window
|
||||
// auth budget would have covered GET /api/auth/session and POST
|
||||
// /api/auth/password-strength, which the frontend calls far more often than
|
||||
// five times per window. authRateLimitGate matches exact method + path
|
||||
// instead, and counts only failed attempts.
|
||||
}
|
||||
|
||||
// Rate limiting for /api. Registered HERE — above the routers — because
|
||||
@@ -525,6 +521,12 @@ async function initializeRateLimiters() {
|
||||
// counted, which matters because monitors poll them every couple of seconds.
|
||||
app.use(createApiRateLimitGate(() => generalRateLimiter));
|
||||
|
||||
// Per-IP limit for credential-verification endpoints only, on its own bucket.
|
||||
// Registered after the general gate so that an IP already over the /api budget
|
||||
// is rejected there first; see authRateLimitGate for the exact endpoint table
|
||||
// and why it must stay unmounted.
|
||||
app.use(createAuthRateLimitGate(() => authRateLimiter));
|
||||
|
||||
app.use(express.json({ limit: '50mb' }));
|
||||
app.use(express.urlencoded({ extended: true, limit: '50mb' }));
|
||||
|
||||
|
||||
Reference in New Issue
Block a user